From 1bdb045a12dd5e40e7f9135e92018b014b94bfa9 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Tue, 5 Mar 2024 18:28:40 -0500 Subject: [PATCH] Enable automatic upgrade server by default (#38937) * Enable automatic upgrade server by default This commit means the proxy now serves automatic upgrade channels by default. The default channel is "default" and its version is the teleport proxy binary version. This commit also fixes a bug with the automatic update version server returning `teleport.Version` instead of `"v"+teleport.Version` when the version was too high. This was not a critical issue because agents are supposed to tolerate versions not prefixed with 'v'. * lint * thread safety * Update lib/automaticupgrades/channel.go * Update lib/config/configuration.go Co-authored-by: Noah Stride * fix tests and lint --------- Co-authored-by: Noah Stride --- integration/proxy/automaticupgrades_test.go | 61 +++++++++- lib/automaticupgrades/channel.go | 83 +++++++++----- lib/automaticupgrades/channel_test.go | 108 ++++++++++-------- lib/config/configuration.go | 6 + lib/config/configuration_test.go | 12 +- lib/srv/discovery/kube_integration_watcher.go | 2 +- lib/web/apiserver.go | 7 -- lib/web/apiserver_test.go | 4 +- 8 files changed, 186 insertions(+), 97 deletions(-) diff --git a/integration/proxy/automaticupgrades_test.go b/integration/proxy/automaticupgrades_test.go index 28fa4d3cd49..7620bb9077c 100644 --- a/integration/proxy/automaticupgrades_test.go +++ b/integration/proxy/automaticupgrades_test.go @@ -33,7 +33,6 @@ import ( "github.com/stretchr/testify/require" "github.com/gravitational/teleport" - "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/integration/helpers" "github.com/gravitational/teleport/lib/automaticupgrades" "github.com/gravitational/teleport/lib/automaticupgrades/basichttp" @@ -43,8 +42,7 @@ import ( ) func createProxyWithChannels(t *testing.T, channels automaticupgrades.Channels) string { - features := proto.Features{} - require.NoError(t, channels.CheckAndSetDefaults(features)) + require.NoError(t, channels.CheckAndSetDefaults()) testDir := t.TempDir() cfg := helpers.InstanceConfig{ @@ -142,7 +140,7 @@ func TestVersionServer(t *testing.T) { { name: "static version too high", channel: staticHighChannel, - expectedResponse: teleport.Version, + expectedResponse: "v" + teleport.Version, }, { name: "static version none", @@ -157,7 +155,7 @@ func TestVersionServer(t *testing.T) { { name: "forward version too high", channel: forwardHighChannel, - expectedResponse: teleport.Version, + expectedResponse: "v" + teleport.Version, }, { name: "forward version none", @@ -187,3 +185,56 @@ func TestVersionServer(t *testing.T) { }) } } +func TestDefaultVersionServer(t *testing.T) { + // Test setup + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + channels := automaticupgrades.Channels{} + + proxyAddr := createProxyWithChannels(t, channels) + + tr := &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + } + httpClient := http.Client{Transport: tr} + + // Test execution + tests := []struct { + name string + channel string + expectedResponse string + }{ + { + name: "default channel is served", + channel: automaticupgrades.DefaultChannelName, + expectedResponse: "v" + teleport.Version, + }, + { + name: "cloud default channel is served", + channel: automaticupgrades.DefaultCloudChannelName, + expectedResponse: "v" + teleport.Version, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + channelUrl, err := url.Parse( + fmt.Sprintf("https://%s/v1/webapi/automaticupgrades/channel/%s/version", proxyAddr, tt.channel), + ) + require.NoError(t, err) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, channelUrl.String(), nil) + require.NoError(t, err) + res, err := httpClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() + + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + + require.Equal(t, http.StatusOK, res.StatusCode) + require.Equal(t, tt.expectedResponse, string(body)) + }) + } +} diff --git a/lib/automaticupgrades/channel.go b/lib/automaticupgrades/channel.go index ffabfbf3540..275f8d93b69 100644 --- a/lib/automaticupgrades/channel.go +++ b/lib/automaticupgrades/channel.go @@ -26,10 +26,10 @@ import ( "sync" "github.com/gravitational/trace" + log "github.com/sirupsen/logrus" "golang.org/x/mod/semver" "github.com/gravitational/teleport" - "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/lib/automaticupgrades/maintenance" "github.com/gravitational/teleport/lib/automaticupgrades/version" ) @@ -45,35 +45,38 @@ type Channels map[string]*Channel // CheckAndSetDefaults checks that every Channel is valid and initializes them. // It also creates default channels if they are not already present. -// Cloud must have the `default` and `stable/cloud` channels. -// Self-hosted with automatic upgrades must have the `default` channel. -func (c Channels) CheckAndSetDefaults(features proto.Features) error { +func (c Channels) CheckAndSetDefaults() error { defaultChannel, err := NewDefaultChannel() if err != nil { return trace.Wrap(err) } - // If we're on cloud, we need at least "cloud/stable" and "default" - if features.GetCloud() { - if _, ok := c[DefaultCloudChannelName]; !ok { - c[DefaultCloudChannelName] = defaultChannel - } - if _, ok := c[DefaultChannelName]; !ok { - c[DefaultChannelName] = c[DefaultCloudChannelName] - } + // Create the "default" channel + + // If the default channel is specified in the config, we use it. + // Else if cloud/stable channel is specified in the config, we use it as default. + // Else, we build a default channel based on the teleport binary version. + if _, ok := c[DefaultChannelName]; ok { + log.Debugln("'default' automatic update channel manually specified, honoring it.") + } else if cloudDefaultChannel, ok := c[DefaultCloudChannelName]; ok { + log.Debugln("'default' automatic update channel not specified, but 'stable/cloud' is, using the cloud default channel by default.") + c[DefaultChannelName] = cloudDefaultChannel + } else { + log.Debugln("'default' automatic update channel not specified, teleport will serve its version by default.") + c[DefaultChannelName] = defaultChannel } - // If we're on self-hosted with automatic upgrades, we need a "default" channel - // We don't want to break existing setups so we'll automatically point to the - // `cloud/stable` channel. - // TODO: in v15 make this a hard requirement and error if `default` is not set - // and automatic upgrades are enabled - if features.GetAutomaticUpgrades() { - if _, ok := c[DefaultChannelName]; !ok { - c[DefaultChannelName] = defaultChannel - } + // Create the "stable/cloud" channel + + // At this point, we know that we have a default channel + // If we don't already have a "stable/cloud" channel we create one based on + // the default one (for compatibility with old updaters). + if _, ok := c[DefaultCloudChannelName]; !ok { + c[DefaultCloudChannelName] = c[DefaultChannelName] } + // Checking each channel. We'll double-check the 'default' one, but + // channel.CheckAndSetDefaults is idempotent. var errs []error for name, channel := range c { // Wrapping is not mandatory here, but it adds the channel name in the @@ -114,12 +117,16 @@ type Channel struct { // teleportMajor stores the current teleport major for comparison. // This field is initialized during CheckAndSetDefaults. teleportMajor int + // mutex protects versionGetter, criticalTrigger, and teleportMajor + mutex sync.Mutex } // CheckAndSetDefaults checks that the Channel configuration is valid and inits // the version getter and maintenance trigger of the Channel based on its // configuration. This function must be called before using the channel. func (c *Channel) CheckAndSetDefaults() error { + c.mutex.Lock() + defer c.mutex.Unlock() switch { case c.ForwardURL != "" && (c.StaticVersion != "" || c.Critical): return trace.BadParameter("cannot set both ForwardURL and (StaticVersion or Critical)") @@ -154,6 +161,8 @@ func (c *Channel) CheckAndSetDefaults() error { // If the version source intentionally did not specify a version, a // NoNewVersionError is returned. func (c *Channel) GetVersion(ctx context.Context) (string, error) { + c.mutex.Lock() + defer c.mutex.Unlock() targetVersion, err := c.versionGetter.GetVersion(ctx) if err != nil { return "", trace.Wrap(err) @@ -167,7 +176,10 @@ func (c *Channel) GetVersion(ctx context.Context) (string, error) { // The target version is officially incompatible with our version, // we prefer returning our version rather than having a broken client if targetMajor > c.teleportMajor { - return teleport.Version, nil + targetVersion, err = version.EnsureSemver(teleport.Version) + if err != nil { + return "", trace.Wrap(err, "ensuring current teleport version is semver-compatible") + } } return targetVersion, nil @@ -176,30 +188,39 @@ func (c *Channel) GetVersion(ctx context.Context) (string, error) { // GetCritical returns the current criticality of the channel. If io is involved, // this function implements cache and is safe to call frequently. func (c *Channel) GetCritical(ctx context.Context) (bool, error) { + c.mutex.Lock() + defer c.mutex.Unlock() return c.criticalTrigger.CanStart(ctx, nil) } var newDefaultChannel = sync.OnceValues[*Channel, error]( func() (*Channel, error) { - forwardURL := GetChannel() - if forwardURL == "" { - forwardURL = stableCloudVersionBaseURL + var channel *Channel + if forwardURL := GetChannel(); forwardURL != "" { + channel = &Channel{ + ForwardURL: forwardURL, + } + } else { + channel = &Channel{ + StaticVersion: teleport.Version, + } } - defaultChannel := &Channel{ - ForwardURL: forwardURL, - } - if err := defaultChannel.CheckAndSetDefaults(); err != nil { + if err := channel.CheckAndSetDefaults(); err != nil { return nil, trace.Wrap(err) } - return defaultChannel, nil + return channel, nil }) // NewDefaultChannel creates a default automatic upgrade channel // It looks up the TELEPORT_AUTOMATIC_UPGRADES_CHANNEL environment variable for -// backward compatibility, and if not found uses the default base URL. +// backward compatibility, and if not found uses binary version. // This default channel can be used in the proxy (to back its own version server) // or in other Teleport processes such as integration services deploying and // updating teleport agents. +// Pre-release versions such as 1.2.3-testbuild.1 will be served AS-IS. +// If you run a test teleport release, agents will attempt to install +// the same release. If you don't want this to happen, you must set the +// 'default' channel to a static version of your choice. func NewDefaultChannel() (*Channel, error) { return newDefaultChannel() } diff --git a/lib/automaticupgrades/channel_test.go b/lib/automaticupgrades/channel_test.go index e69e1a6ec3b..8a32951822e 100644 --- a/lib/automaticupgrades/channel_test.go +++ b/lib/automaticupgrades/channel_test.go @@ -25,7 +25,6 @@ import ( "github.com/stretchr/testify/require" "github.com/gravitational/teleport" - "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/lib/automaticupgrades/constants" "github.com/gravitational/teleport/lib/automaticupgrades/maintenance" "github.com/gravitational/teleport/lib/automaticupgrades/version" @@ -34,26 +33,36 @@ import ( const testVersion = "v1.2.3" func Test_Channels_CheckAndSetDefaults(t *testing.T) { - noFeatures := proto.Features{} - cloudFeatures := proto.Features{Cloud: true, AutomaticUpgrades: true} customChannelURL := "https://foo.example.com/bar" t.Run("no channels", func(t *testing.T) { + // When we start without channels, two channels must be created: + // - "default" + // - "stable/cloud" c := Channels{} - require.NoError(t, c.CheckAndSetDefaults(noFeatures)) + require.NoError(t, c.CheckAndSetDefaults()) + require.Len(t, c, 2) }) t.Run("single channel", func(t *testing.T) { + // When we start with a channel, we must keep it and create "default" + // and "stable/cloud". + // The channel we passed must also get initialized. channel := &Channel{StaticVersion: testVersion} c := Channels{"foo": channel} - require.NoError(t, c.CheckAndSetDefaults(noFeatures)) + require.NoError(t, c.CheckAndSetDefaults()) + require.Len(t, c, 3) require.NotNil(t, channel.versionGetter) require.NotNil(t, channel.criticalTrigger) }) t.Run("many channels", func(t *testing.T) { + // When we start with many channels, we must keep them and create "default" + // and "stable/cloud". + // The channels passed must also get initialized. channel1 := &Channel{StaticVersion: testVersion} channel2 := &Channel{StaticVersion: testVersion} channel3 := &Channel{StaticVersion: testVersion} c := Channels{"foo": channel1, "bar": channel2, "baz": channel3} - require.NoError(t, c.CheckAndSetDefaults(noFeatures)) + require.NoError(t, c.CheckAndSetDefaults()) + require.Len(t, c, 5) require.NotNil(t, channel1.versionGetter) require.NotNil(t, channel1.criticalTrigger) require.NotNil(t, channel2.versionGetter) @@ -61,10 +70,12 @@ func Test_Channels_CheckAndSetDefaults(t *testing.T) { require.NotNil(t, channel3.versionGetter) require.NotNil(t, channel3.criticalTrigger) }) - t.Run("default channels for cloud", func(t *testing.T) { - // Cloud must have `default` and `stable/cloud` channels by default - c := Channels{} - require.NoError(t, c.CheckAndSetDefaults(cloudFeatures)) + t.Run("stable/cloud set but not default", func(t *testing.T) { + // When "stable/cloud" is set but not "default", we must use "stable/cloud" as "default". + c := Channels{ + DefaultCloudChannelName: &Channel{ForwardURL: stableCloudVersionBaseURL}, + } + require.NoError(t, c.CheckAndSetDefaults()) require.Len(t, c, 2) defaultChannel, ok := c[DefaultChannelName] require.True(t, ok) @@ -73,11 +84,27 @@ func Test_Channels_CheckAndSetDefaults(t *testing.T) { require.True(t, ok) require.Equal(t, stableCloudVersionBaseURL, stableCloudChannel.ForwardURL) }) - t.Run("cloud override stable/cloud", func(t *testing.T) { - // When "stable/cloud" channel is configured, CheckAndSetDefaults - // must honor it AND also use it as the "default" channel. - c := Channels{DefaultCloudChannelName: &Channel{ForwardURL: customChannelURL}} - require.NoError(t, c.CheckAndSetDefaults(cloudFeatures)) + t.Run("default set but not stable/cloud", func(t *testing.T) { + // When "default" is set but not "stable/cloud", we must use "default" as "stable/cloud". + c := Channels{ + DefaultChannelName: &Channel{ForwardURL: stableCloudVersionBaseURL}, + } + require.NoError(t, c.CheckAndSetDefaults()) + require.Len(t, c, 2) + defaultChannel, ok := c[DefaultChannelName] + require.True(t, ok) + require.Equal(t, stableCloudVersionBaseURL, defaultChannel.ForwardURL) + stableCloudChannel, ok := c[DefaultCloudChannelName] + require.True(t, ok) + require.Equal(t, stableCloudVersionBaseURL, stableCloudChannel.ForwardURL) + }) + t.Run("stable/cloud and default set", func(t *testing.T) { + // When both "stable/cloud" and "default" are set we must not change them. + c := Channels{ + DefaultChannelName: &Channel{ForwardURL: customChannelURL}, + DefaultCloudChannelName: &Channel{ForwardURL: customChannelURL}, + } + require.NoError(t, c.CheckAndSetDefaults()) require.Len(t, c, 2) stableCloudChannel, ok := c[DefaultCloudChannelName] require.True(t, ok) @@ -86,53 +113,25 @@ func Test_Channels_CheckAndSetDefaults(t *testing.T) { require.True(t, ok) require.Equal(t, customChannelURL, defaultChannel.ForwardURL) }) - t.Run("cloud override default", func(t *testing.T) { - // When the "default" channel is manually configured, CheckAndSetDefaults - // must honor it. - // In this test, only the "default" channel must be custom, the - // "stable/cloud" channel must point to the standard cloud URL. - c := Channels{DefaultChannelName: &Channel{ForwardURL: customChannelURL}} - require.NoError(t, c.CheckAndSetDefaults(cloudFeatures)) - require.Len(t, c, 2) - defaultChannel, ok := c[DefaultChannelName] - require.True(t, ok) - require.Equal(t, customChannelURL, defaultChannel.ForwardURL) - stableCloudChannel, ok := c[DefaultCloudChannelName] - require.True(t, ok) - require.Equal(t, stableCloudVersionBaseURL, stableCloudChannel.ForwardURL) - }) - t.Run("self-hosted no channel", func(t *testing.T) { - // In self-hosted automatic-upgrades setups, we need a default channel. - // For backward compatibility we should add it instead of requiring it. - c := Channels{} - require.NoError(t, c.CheckAndSetDefaults(proto.Features{AutomaticUpgrades: true})) - require.Len(t, c, 1) - defaultChannel, ok := c[DefaultChannelName] - require.True(t, ok) - require.Equal(t, stableCloudVersionBaseURL, defaultChannel.ForwardURL) - _, ok = c[DefaultCloudChannelName] - require.False(t, ok) - }) - } func Test_Channel_CheckAndSetDefaults(t *testing.T) { tests := []struct { name string - channel Channel + channel *Channel assertError require.ErrorAssertionFunc expectedVersionGetterType interface{} expectedCriticalTriggerType interface{} }{ { name: "empty (invalid)", - channel: Channel{}, + channel: &Channel{}, assertError: require.Error, }, { name: "forward URL (valid)", - channel: Channel{ + channel: &Channel{ ForwardURL: stableCloudVersionBaseURL, }, assertError: require.NoError, @@ -141,7 +140,7 @@ func Test_Channel_CheckAndSetDefaults(t *testing.T) { }, { name: "static version (valid)", - channel: Channel{ + channel: &Channel{ StaticVersion: testVersion, }, assertError: require.NoError, @@ -150,7 +149,7 @@ func Test_Channel_CheckAndSetDefaults(t *testing.T) { }, { name: "all set (invalid)", - channel: Channel{ + channel: &Channel{ ForwardURL: stableCloudVersionBaseURL, StaticVersion: testVersion, }, @@ -189,7 +188,7 @@ func Test_Channel_GetVersion(t *testing.T) { { name: "version too high", targetVersion: "v99.1.1", - expectedVersion: teleport.Version, + expectedVersion: "v" + teleport.Version, assertErr: require.NoError, }, { @@ -209,3 +208,14 @@ func Test_Channel_GetVersion(t *testing.T) { }) } } + +func TestNewDefaultChannel(t *testing.T) { + channel, err := NewDefaultChannel() + require.NoError(t, err) + // Default channel must return teleport version + require.Equal(t, teleport.Version, channel.StaticVersion) + require.False(t, channel.Critical) + // And the default channel must be initialized + require.NotNil(t, channel.versionGetter) + require.NotNil(t, channel.criticalTrigger) +} diff --git a/lib/config/configuration.go b/lib/config/configuration.go index dc5e7fd56a9..df0b50f06d0 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -51,6 +51,7 @@ import ( "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib" + "github.com/gravitational/teleport/lib/automaticupgrades" "github.com/gravitational/teleport/lib/backend" "github.com/gravitational/teleport/lib/backend/lite" "github.com/gravitational/teleport/lib/backend/memory" @@ -1163,6 +1164,11 @@ func applyProxyConfig(fc *FileConfig, cfg *servicecfg.Config) error { if fc.Proxy.AutomaticUpgradesChannels != nil { cfg.Proxy.AutomaticUpgradesChannels = fc.Proxy.AutomaticUpgradesChannels + } else { + cfg.Proxy.AutomaticUpgradesChannels = make(automaticupgrades.Channels) + } + if err = cfg.Proxy.AutomaticUpgradesChannels.CheckAndSetDefaults(); err != nil { + return trace.Wrap(err, "validating the automatic upgrades configuration") } // This is the legacy format. Continue to support it forever, but ideally diff --git a/lib/config/configuration_test.go b/lib/config/configuration_test.go index f7d37b96b0d..ba0ebce8568 100644 --- a/lib/config/configuration_test.go +++ b/lib/config/configuration_test.go @@ -1783,7 +1783,11 @@ func TestSetDefaultListenerAddresses(t *testing.T) { require.NoError(t, ApplyFileConfig(&tt.fc, cfg)) require.NoError(t, Configure(&CommandLineFlags{}, cfg, false)) - require.Empty(t, cmp.Diff(cfg.Proxy, tt.want, cmpopts.EquateEmpty())) + opts := cmp.Options{ + cmpopts.EquateEmpty(), + cmpopts.IgnoreFields(servicecfg.ProxyConfig{}, "AutomaticUpgradesChannels"), + } + require.Empty(t, cmp.Diff(cfg.Proxy, tt.want, opts...)) }) } } @@ -2113,7 +2117,11 @@ func TestProxyConfigurationVersion(t *testing.T) { cfg := servicecfg.MakeDefaultConfig() err := ApplyFileConfig(&tt.fc, cfg) tt.checkErr(t, err) - require.Empty(t, cmp.Diff(cfg.Proxy, tt.want, cmpopts.EquateEmpty())) + opts := cmp.Options{ + cmpopts.EquateEmpty(), + cmpopts.IgnoreFields(servicecfg.ProxyConfig{}, "AutomaticUpgradesChannels"), + } + require.Empty(t, cmp.Diff(cfg.Proxy, tt.want, opts...)) }) } } diff --git a/lib/srv/discovery/kube_integration_watcher.go b/lib/srv/discovery/kube_integration_watcher.go index 1d76220e0bf..cee614f4a9f 100644 --- a/lib/srv/discovery/kube_integration_watcher.go +++ b/lib/srv/discovery/kube_integration_watcher.go @@ -48,7 +48,7 @@ func (s *Server) startKubeIntegrationWatchers() error { clt := s.AccessPoint releaseChannels := automaticupgrades.Channels{} - if err := releaseChannels.CheckAndSetDefaults(s.ClusterFeatures()); err != nil { + if err := releaseChannels.CheckAndSetDefaults(); err != nil { return trace.Wrap(err) } diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 84c72e62a53..9719ab6a3aa 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -394,13 +394,6 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { h.cfg.AutomaticUpgradesChannels = automaticupgrades.Channels{} } - if h.cfg.AutomaticUpgradesChannels != nil { - err := h.cfg.AutomaticUpgradesChannels.CheckAndSetDefaults(cfg.ClusterFeatures) - if err != nil { - return nil, trace.Wrap(err) - } - } - // for properly handling url-encoded parameter values. h.UseRawPath = true diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index e341254ad5d..d4b17a3ac48 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4554,13 +4554,13 @@ func TestGetWebConfig(t *testing.T) { StaticVersion: testVersion, }, } - require.NoError(t, channels.CheckAndSetDefaults(authproto.Features{AutomaticUpgrades: true, Cloud: true})) + require.NoError(t, channels.CheckAndSetDefaults()) env.proxies[0].handler.handler.cfg.AutomaticUpgradesChannels = channels expectedCfg.IsCloud = true expectedCfg.IsUsageBasedBilling = true expectedCfg.AutomaticUpgrades = true - expectedCfg.AutomaticUpgradesTargetVersion = teleport.Version + expectedCfg.AutomaticUpgradesTargetVersion = "v" + teleport.Version expectedCfg.AssistEnabled = false // request and verify enabled features are enabled.