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 <noah.stride@goteleport.com>

* fix tests and lint

---------

Co-authored-by: Noah Stride <noah.stride@goteleport.com>

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))
+		})
+	}
+}

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()
 }

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)
+}

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

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...))
 		})
 	}
 }

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)
 	}
 

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
 

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.