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.