From aa70c2b8ff471a8e3d3ee9709b1ba766edf2c694 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Tue, 27 Aug 2024 17:05:26 -0300 Subject: [PATCH 01/28] Add feature watcher --- entitlements/entitlements.go | 66 ++++++++++++++++++++++++++++++ lib/service/connect.go | 66 +----------------------------- lib/web/apiserver.go | 31 +++++++++----- lib/web/features.go | 72 +++++++++++++++++++++++++++++++++ lib/web/integrations_awsoidc.go | 6 +-- lib/web/join_tokens.go | 2 +- lib/web/server.go | 2 + 7 files changed, 165 insertions(+), 80 deletions(-) create mode 100644 lib/web/features.go diff --git a/entitlements/entitlements.go b/entitlements/entitlements.go index afde35c8c4f1..2809981afe29 100644 --- a/entitlements/entitlements.go +++ b/entitlements/entitlements.go @@ -16,6 +16,8 @@ package entitlements +import "github.com/gravitational/teleport/api/client/proto" + type EntitlementKind string // The EntitlementKind list should be 1:1 with the Features & FeatureStrings in salescenter/product/product.go, @@ -56,3 +58,67 @@ var AllEntitlements = []EntitlementKind{ ExternalAuditStorage, FeatureHiding, HSM, Identity, JoinActiveSessions, K8s, MobileDeviceManagement, OIDC, OktaSCIM, OktaUserSync, Policy, SAML, SessionLocks, UpsellAlert, UsageReporting, } + +// supportEntitlementsCompatibility ensures entitlements are backwards compatible +// If Entitlements are present, there are no changes +// If Entitlements are not present, sets the entitlements fields to legacy field values +// TODO(michellescripts) remove in v18 +func SupportEntitlementsCompatibility(features *proto.Features) { + if len(features.Entitlements) > 0 { + return + } + + features.Entitlements = getBaseEntitlements(features.GetEntitlements()) + + // Entitlements: All records are {enabled: false}; update to equal legacy feature value + features.Entitlements[string(ExternalAuditStorage)] = &proto.EntitlementInfo{Enabled: features.GetExternalAuditStorage()} + features.Entitlements[string(FeatureHiding)] = &proto.EntitlementInfo{Enabled: features.GetFeatureHiding()} + features.Entitlements[string(Identity)] = &proto.EntitlementInfo{Enabled: features.GetIdentityGovernance()} + features.Entitlements[string(JoinActiveSessions)] = &proto.EntitlementInfo{Enabled: features.GetJoinActiveSessions()} + features.Entitlements[string(MobileDeviceManagement)] = &proto.EntitlementInfo{Enabled: features.GetMobileDeviceManagement()} + features.Entitlements[string(OIDC)] = &proto.EntitlementInfo{Enabled: features.GetOIDC()} + features.Entitlements[string(Policy)] = &proto.EntitlementInfo{Enabled: features.GetPolicy().GetEnabled()} + features.Entitlements[string(SAML)] = &proto.EntitlementInfo{Enabled: features.GetSAML()} + features.Entitlements[string(K8s)] = &proto.EntitlementInfo{Enabled: features.GetKubernetes()} + features.Entitlements[string(App)] = &proto.EntitlementInfo{Enabled: features.GetApp()} + features.Entitlements[string(DB)] = &proto.EntitlementInfo{Enabled: features.GetDB()} + features.Entitlements[string(Desktop)] = &proto.EntitlementInfo{Enabled: features.GetDesktop()} + features.Entitlements[string(HSM)] = &proto.EntitlementInfo{Enabled: features.GetHSM()} + + // set default Identity fields to legacy feature value + features.Entitlements[string(AccessLists)] = &proto.EntitlementInfo{Enabled: true, Limit: features.GetAccessList().GetCreateLimit()} + features.Entitlements[string(AccessMonitoring)] = &proto.EntitlementInfo{Enabled: features.GetAccessMonitoring().GetEnabled(), Limit: features.GetAccessMonitoring().GetMaxReportRangeLimit()} + features.Entitlements[string(AccessRequests)] = &proto.EntitlementInfo{Enabled: features.GetAccessRequests().MonthlyRequestLimit > 0, Limit: features.GetAccessRequests().GetMonthlyRequestLimit()} + features.Entitlements[string(DeviceTrust)] = &proto.EntitlementInfo{Enabled: features.GetDeviceTrust().GetEnabled(), Limit: features.GetDeviceTrust().GetDevicesUsageLimit()} + // override Identity Package features if Identity is enabled: set true and clear limit + if features.GetIdentityGovernance() { + features.Entitlements[string(AccessLists)] = &proto.EntitlementInfo{Enabled: true} + features.Entitlements[string(AccessMonitoring)] = &proto.EntitlementInfo{Enabled: true} + features.Entitlements[string(AccessRequests)] = &proto.EntitlementInfo{Enabled: true} + features.Entitlements[string(DeviceTrust)] = &proto.EntitlementInfo{Enabled: true} + features.Entitlements[string(OktaSCIM)] = &proto.EntitlementInfo{Enabled: true} + features.Entitlements[string(OktaUserSync)] = &proto.EntitlementInfo{Enabled: true} + features.Entitlements[string(SessionLocks)] = &proto.EntitlementInfo{Enabled: true} + } +} + +// getBaseEntitlements takes a cloud entitlement set and returns a modules Entitlement set +func getBaseEntitlements(protoEntitlements map[string]*proto.EntitlementInfo) map[string]*proto.EntitlementInfo { + all := AllEntitlements + result := make(map[string]*proto.EntitlementInfo, len(all)) + + for _, e := range all { + al, ok := protoEntitlements[string(e)] + if !ok { + result[string(e)] = &proto.EntitlementInfo{} + continue + } + + result[string(e)] = &proto.EntitlementInfo{ + Enabled: al.Enabled, + Limit: al.Limit, + } + } + + return result +} diff --git a/lib/service/connect.go b/lib/service/connect.go index 8f275e5ec471..0e99a6f7f7a7 100644 --- a/lib/service/connect.go +++ b/lib/service/connect.go @@ -1130,7 +1130,7 @@ func (process *TeleportProcess) getConnector(clientIdentity, serverIdentity *sta // Set cluster features and return successfully with a working connector. // TODO(michellescripts) remove clone & compatibility check in v18 cloned := apiutils.CloneProtoMsg(pingResponse.GetServerFeatures()) - supportEntitlementsCompatibility(cloned) + entitlements.SupportEntitlementsCompatibility(cloned) process.setClusterFeatures(cloned) process.setAuthSubjectiveAddr(pingResponse.RemoteAddr) process.logger.InfoContext(process.ExitContext(), "features loaded from auth server", "identity", clientIdentity.ID.Role, "features", pingResponse.GetServerFeatures()) @@ -1139,70 +1139,6 @@ func (process *TeleportProcess) getConnector(clientIdentity, serverIdentity *sta return newConn, nil } -// supportEntitlementsCompatibility ensures entitlements are backwards compatible -// If Entitlements are present, there are no changes -// If Entitlements are not present, sets the entitlements fields to legacy field values -// TODO(michellescripts) remove in v18 -func supportEntitlementsCompatibility(features *proto.Features) { - if len(features.Entitlements) > 0 { - return - } - - features.Entitlements = getBaseEntitlements(features.GetEntitlements()) - - // Entitlements: All records are {enabled: false}; update to equal legacy feature value - features.Entitlements[string(entitlements.ExternalAuditStorage)] = &proto.EntitlementInfo{Enabled: features.GetExternalAuditStorage()} - features.Entitlements[string(entitlements.FeatureHiding)] = &proto.EntitlementInfo{Enabled: features.GetFeatureHiding()} - features.Entitlements[string(entitlements.Identity)] = &proto.EntitlementInfo{Enabled: features.GetIdentityGovernance()} - features.Entitlements[string(entitlements.JoinActiveSessions)] = &proto.EntitlementInfo{Enabled: features.GetJoinActiveSessions()} - features.Entitlements[string(entitlements.MobileDeviceManagement)] = &proto.EntitlementInfo{Enabled: features.GetMobileDeviceManagement()} - features.Entitlements[string(entitlements.OIDC)] = &proto.EntitlementInfo{Enabled: features.GetOIDC()} - features.Entitlements[string(entitlements.Policy)] = &proto.EntitlementInfo{Enabled: features.GetPolicy().GetEnabled()} - features.Entitlements[string(entitlements.SAML)] = &proto.EntitlementInfo{Enabled: features.GetSAML()} - features.Entitlements[string(entitlements.K8s)] = &proto.EntitlementInfo{Enabled: features.GetKubernetes()} - features.Entitlements[string(entitlements.App)] = &proto.EntitlementInfo{Enabled: features.GetApp()} - features.Entitlements[string(entitlements.DB)] = &proto.EntitlementInfo{Enabled: features.GetDB()} - features.Entitlements[string(entitlements.Desktop)] = &proto.EntitlementInfo{Enabled: features.GetDesktop()} - features.Entitlements[string(entitlements.HSM)] = &proto.EntitlementInfo{Enabled: features.GetHSM()} - - // set default Identity fields to legacy feature value - features.Entitlements[string(entitlements.AccessLists)] = &proto.EntitlementInfo{Enabled: true, Limit: features.GetAccessList().GetCreateLimit()} - features.Entitlements[string(entitlements.AccessMonitoring)] = &proto.EntitlementInfo{Enabled: features.GetAccessMonitoring().GetEnabled(), Limit: features.GetAccessMonitoring().GetMaxReportRangeLimit()} - features.Entitlements[string(entitlements.AccessRequests)] = &proto.EntitlementInfo{Enabled: features.GetAccessRequests().MonthlyRequestLimit > 0, Limit: features.GetAccessRequests().GetMonthlyRequestLimit()} - features.Entitlements[string(entitlements.DeviceTrust)] = &proto.EntitlementInfo{Enabled: features.GetDeviceTrust().GetEnabled(), Limit: features.GetDeviceTrust().GetDevicesUsageLimit()} - // override Identity Package features if Identity is enabled: set true and clear limit - if features.GetIdentityGovernance() { - features.Entitlements[string(entitlements.AccessLists)] = &proto.EntitlementInfo{Enabled: true} - features.Entitlements[string(entitlements.AccessMonitoring)] = &proto.EntitlementInfo{Enabled: true} - features.Entitlements[string(entitlements.AccessRequests)] = &proto.EntitlementInfo{Enabled: true} - features.Entitlements[string(entitlements.DeviceTrust)] = &proto.EntitlementInfo{Enabled: true} - features.Entitlements[string(entitlements.OktaSCIM)] = &proto.EntitlementInfo{Enabled: true} - features.Entitlements[string(entitlements.OktaUserSync)] = &proto.EntitlementInfo{Enabled: true} - features.Entitlements[string(entitlements.SessionLocks)] = &proto.EntitlementInfo{Enabled: true} - } -} - -// getBaseEntitlements takes a cloud entitlement set and returns a modules Entitlement set -func getBaseEntitlements(protoEntitlements map[string]*proto.EntitlementInfo) map[string]*proto.EntitlementInfo { - all := entitlements.AllEntitlements - result := make(map[string]*proto.EntitlementInfo, len(all)) - - for _, e := range all { - al, ok := protoEntitlements[string(e)] - if !ok { - result[string(e)] = &proto.EntitlementInfo{} - continue - } - - result[string(e)] = &proto.EntitlementInfo{ - Enabled: al.Enabled, - Limit: al.Limit, - } - } - - return result -} - // newClient attempts to connect to either the proxy server or auth server // For config v3 and onwards, it will only connect to either the proxy (via tunnel) or the auth server (direct), // depending on what was specified in the config. diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index e232a606927a..3715a0270804 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -125,6 +125,9 @@ const ( IncludedResourceModeRequestable = "requestable" // IncludedResourceModeAll describes that all resources, requestable and available, should be returned. IncludedResourceModeAll = "all" + // DefaultLicenseWatchInterval is the default time in which the license watcher + // should ping the auth server for new cluster features + DefaultLicenseWatchInterval = time.Second * 1 // time.Minute * 2 ) // healthCheckAppServerFunc defines a function used to perform a health check @@ -154,12 +157,12 @@ type Handler struct { // userConns tracks amount of current active connections with user certificates. userConns atomic.Int32 - // ClusterFeatures contain flags for supported and unsupported features. + // clusterFeatures contain flags for supported and unsupported features. // Note: This field can become stale since it's only set on initial proxy // startup. To get the latest feature flags you'll need to ping from the // auth server. // https://github.com/gravitational/teleport/issues/39161 - ClusterFeatures proto.Features + clusterFeatures proto.Features // nodeWatcher is a services.NodeWatcher used by Assist to lookup nodes from // the proxy's cache and get nodes in real time. @@ -172,6 +175,10 @@ type Handler struct { // an authenticated websocket so unauthenticated sockets dont get left // open. wsIODeadline time.Duration + + // featureWatcherStop is a channel used to emit a stop signal to the + // license watcher goroutine + featureWatcherStop chan struct{} } // HandlerOption is a functional argument - an option that can be passed @@ -315,6 +322,10 @@ type Config struct { // IntegrationAppHandler handles App Access requests which use an Integration. IntegrationAppHandler app.ServerHandler + + // LicenseWatchInterval is the interval between pings to the auth server + // to fetch new cluster features + LicenseWatchInterval time.Duration } // SetDefaults ensures proper default values are set if @@ -329,6 +340,10 @@ func (c *Config) SetDefaults() { if c.PresenceChecker == nil { c.PresenceChecker = client.RunPresenceTask } + + if c.LicenseWatchInterval == 0 { + c.LicenseWatchInterval = DefaultLicenseWatchInterval + } } type APIHandler struct { @@ -385,10 +400,11 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { log: newPackageLogger(), logger: slog.Default().With(teleport.ComponentKey, teleport.ComponentWeb), clock: clockwork.NewRealClock(), - ClusterFeatures: cfg.ClusterFeatures, + clusterFeatures: cfg.ClusterFeatures, healthCheckAppServer: cfg.HealthCheckAppServer, tracer: cfg.TracerProvider.Tracer(teleport.ComponentWeb), wsIODeadline: wsIODeadline, + featureWatcherStop: make(chan struct{}), } if automaticUpgrades(cfg.ClusterFeatures) && h.cfg.AutomaticUpgradesChannels == nil { @@ -1626,14 +1642,7 @@ func (h *Handler) getWebConfig(w http.ResponseWriter, r *http.Request, p httprou } } - clusterFeatures := h.ClusterFeatures - // ping server to get cluster features since h.ClusterFeatures may be stale - pingResponse, err := h.GetProxyClient().Ping(r.Context()) - if err != nil { - h.log.WithError(err).Warn("Cannot retrieve cluster features, client may receive stale features") - } else { - clusterFeatures = *pingResponse.ServerFeatures - } + clusterFeatures := h.clusterFeatures // get tunnel address to display on cloud instances tunnelPublicAddr := "" diff --git a/lib/web/features.go b/lib/web/features.go new file mode 100644 index 000000000000..d6491fe2dc6e --- /dev/null +++ b/lib/web/features.go @@ -0,0 +1,72 @@ +/* + * Teleport + * Copyright (C) 2023 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +// Package web implements web proxy handler that provides +// web interface to view and connect to teleport nodes +package web + +import ( + "context" + + "github.com/gravitational/teleport/api/client/proto" + "github.com/gravitational/teleport/entitlements" +) + +func (h *Handler) SetClusterFeatures(features proto.Features) { + h.Mutex.Lock() + defer h.Mutex.Unlock() + + entitlements.SupportEntitlementsCompatibility(&features) + h.clusterFeatures = features +} + +func (h *Handler) GetClusterFeatures() proto.Features { + h.Mutex.Lock() + defer h.Mutex.Unlock() + + return h.clusterFeatures +} + +func (h *Handler) startFeaturesWatcher() { + ticker := h.clock.NewTicker(h.cfg.LicenseWatchInterval) + h.log.WithField("interval", h.cfg.LicenseWatchInterval).Info("Proxy handler features watcher has started") + ctx := context.Background() + + defer ticker.Stop() + for { + select { + case <-ticker.Chan(): + h.log.Info("Pinging auth server for features") + f, err := h.GetProxyClient().Ping(ctx) + if err != nil { + h.log.WithError(err).Error("Failed fetching features") + continue + } + + h.SetClusterFeatures(*f.ServerFeatures) + h.log.WithField("features", f.ServerFeatures).Infof("Done updating proxy features: %+v", f) + case <-h.featureWatcherStop: + h.log.Info("Feature service has stopped") + return + } + } +} + +func (h *Handler) stopFeaturesWatcher() { + close(h.featureWatcherStop) +} diff --git a/lib/web/integrations_awsoidc.go b/lib/web/integrations_awsoidc.go index f188327c61f2..47840225295e 100644 --- a/lib/web/integrations_awsoidc.go +++ b/lib/web/integrations_awsoidc.go @@ -148,7 +148,7 @@ func (h *Handler) awsOIDCDeployService(w http.ResponseWriter, r *http.Request, p } teleportVersionTag := teleport.Version - if automaticUpgrades(h.ClusterFeatures) { + if automaticUpgrades(h.GetClusterFeatures()) { cloudStableVersion, err := h.cfg.AutomaticUpgradesChannels.DefaultVersion(ctx) if err != nil { return "", trace.Wrap(err) @@ -201,7 +201,7 @@ func (h *Handler) awsOIDCDeployDatabaseServices(w http.ResponseWriter, r *http.R } teleportVersionTag := teleport.Version - if automaticUpgrades(h.ClusterFeatures) { + if automaticUpgrades(h.GetClusterFeatures()) { cloudStableVersion, err := h.cfg.AutomaticUpgradesChannels.DefaultVersion(ctx) if err != nil { return "", trace.Wrap(err) @@ -503,7 +503,7 @@ func (h *Handler) awsOIDCEnrollEKSClusters(w http.ResponseWriter, r *http.Reques return nil, trace.BadParameter("an integration name is required") } - agentVersion, err := kubeutils.GetKubeAgentVersion(ctx, h.cfg.ProxyClient, h.ClusterFeatures, h.cfg.AutomaticUpgradesChannels) + agentVersion, err := kubeutils.GetKubeAgentVersion(ctx, h.cfg.ProxyClient, h.GetClusterFeatures(), h.cfg.AutomaticUpgradesChannels) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/web/join_tokens.go b/lib/web/join_tokens.go index c5939e30953d..7dcd657857ab 100644 --- a/lib/web/join_tokens.go +++ b/lib/web/join_tokens.go @@ -377,7 +377,7 @@ func (h *Handler) createTokenForDiscoveryHandle(w http.ResponseWriter, r *http.R func (h *Handler) getAutoUpgrades(ctx context.Context) (bool, string, error) { var autoUpgradesVersion string var err error - autoUpgrades := automaticUpgrades(h.ClusterFeatures) + autoUpgrades := automaticUpgrades(h.GetClusterFeatures()) if autoUpgrades { autoUpgradesVersion, err = h.cfg.AutomaticUpgradesChannels.DefaultVersion(ctx) if err != nil { diff --git a/lib/web/server.go b/lib/web/server.go index 0af94831d30e..2542821b9151 100644 --- a/lib/web/server.go +++ b/lib/web/server.go @@ -98,6 +98,7 @@ func (s *Server) Serve(l net.Listener) error { if closed { return trace.Errorf("serve called on previously closed server") } + go s.cfg.Handler.handler.startFeaturesWatcher() return trace.Wrap(s.cfg.Server.Serve(l)) } @@ -131,6 +132,7 @@ func (s *Server) Shutdown(ctx context.Context) error { err = s.ln.Close() } s.mu.Unlock() + s.cfg.Handler.handler.stopFeaturesWatcher() activeConnections := s.cfg.Handler.handler.userConns.Load() if activeConnections == 0 { From b610f4c86fbe3b6181b211daf6609fc1c03a62ae Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 28 Aug 2024 13:10:09 -0300 Subject: [PATCH 02/28] Add test --- lib/web/apiserver.go | 5 +- lib/web/features.go | 40 ++++++----- lib/web/features_test.go | 145 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 21 deletions(-) create mode 100644 lib/web/features_test.go diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 3715a0270804..bf507fd028d6 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -127,7 +127,7 @@ const ( IncludedResourceModeAll = "all" // DefaultLicenseWatchInterval is the default time in which the license watcher // should ping the auth server for new cluster features - DefaultLicenseWatchInterval = time.Second * 1 // time.Minute * 2 + DefaultLicenseWatchInterval = time.Minute * 5 ) // healthCheckAppServerFunc defines a function used to perform a health check @@ -179,6 +179,7 @@ type Handler struct { // featureWatcherStop is a channel used to emit a stop signal to the // license watcher goroutine featureWatcherStop chan struct{} + featureWatcherOnce sync.Once } // HandlerOption is a functional argument - an option that can be passed @@ -1642,7 +1643,7 @@ func (h *Handler) getWebConfig(w http.ResponseWriter, r *http.Request, p httprou } } - clusterFeatures := h.clusterFeatures + clusterFeatures := h.GetClusterFeatures() // get tunnel address to display on cloud instances tunnelPublicAddr := "" diff --git a/lib/web/features.go b/lib/web/features.go index d6491fe2dc6e..f91f80ed1666 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -43,28 +43,30 @@ func (h *Handler) GetClusterFeatures() proto.Features { } func (h *Handler) startFeaturesWatcher() { - ticker := h.clock.NewTicker(h.cfg.LicenseWatchInterval) - h.log.WithField("interval", h.cfg.LicenseWatchInterval).Info("Proxy handler features watcher has started") - ctx := context.Background() + h.featureWatcherOnce.Do(func() { + ticker := h.clock.NewTicker(h.cfg.LicenseWatchInterval) + h.log.WithField("interval", h.cfg.LicenseWatchInterval).Info("Proxy handler features watcher has started") + ctx := context.Background() - defer ticker.Stop() - for { - select { - case <-ticker.Chan(): - h.log.Info("Pinging auth server for features") - f, err := h.GetProxyClient().Ping(ctx) - if err != nil { - h.log.WithError(err).Error("Failed fetching features") - continue - } + defer ticker.Stop() + for { + select { + case <-ticker.Chan(): + h.log.Info("Pinging auth server for features") + f, err := h.cfg.ProxyClient.Ping(ctx) + if err != nil { + h.log.WithError(err).Error("Auth server ping failed") + continue + } - h.SetClusterFeatures(*f.ServerFeatures) - h.log.WithField("features", f.ServerFeatures).Infof("Done updating proxy features: %+v", f) - case <-h.featureWatcherStop: - h.log.Info("Feature service has stopped") - return + h.SetClusterFeatures(*f.ServerFeatures) + h.log.Debug("Done updating proxy features") + case <-h.featureWatcherStop: + h.log.Info("Feature service has stopped") + return + } } - } + }) } func (h *Handler) stopFeaturesWatcher() { diff --git a/lib/web/features_test.go b/lib/web/features_test.go new file mode 100644 index 000000000000..af8ec8ded4bc --- /dev/null +++ b/lib/web/features_test.go @@ -0,0 +1,145 @@ +package web + +import ( + "context" + "log/slog" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/client/proto" + "github.com/gravitational/teleport/api/utils" + "github.com/gravitational/teleport/entitlements" + "github.com/jonboulle/clockwork" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFeaturesWatcher(t *testing.T) { + clock := clockwork.NewFakeClock() + mockedFeatures := proto.Features{ + Kubernetes: true, + Entitlements: map[string]*proto.EntitlementInfo{}, + AccessRequests: &proto.AccessRequestsFeature{}, + } + + handler := &Handler{ + cfg: Config{ + LicenseWatchInterval: 100 * time.Millisecond, + ProxyClient: &mockedPingTestProxy{ + mockedPing: func(ctx context.Context) (proto.PingResponse, error) { + return proto.PingResponse{ + ServerFeatures: &mockedFeatures, + }, nil + }, + }, + }, + clock: clock, + clusterFeatures: proto.Features{}, + featureWatcherStop: make(chan struct{}), + log: newPackageLogger(), + logger: slog.Default().With(teleport.ComponentKey, teleport.ComponentWeb), + } + + // before running the watcher, features should match the value passed to the handler + requireFeatures(t, clock, proto.Features{}, handler.GetClusterFeatures) + + go handler.startFeaturesWatcher() + clock.BlockUntil(1) + + // after starting the watcher, handler.GetClusterFeatures should return + // values matching the client's response + features := proto.Features{ + Kubernetes: true, + Entitlements: map[string]*proto.EntitlementInfo{}, + AccessRequests: &proto.AccessRequestsFeature{}, + } + entitlements.SupportEntitlementsCompatibility(&features) + expected := utils.CloneProtoMsg(&features) + requireFeatures(t, clock, *expected, handler.GetClusterFeatures) + + // update values once again and check if the features are properly updated + features = proto.Features{ + Kubernetes: false, + Entitlements: map[string]*proto.EntitlementInfo{}, + AccessRequests: &proto.AccessRequestsFeature{}, + } + entitlements.SupportEntitlementsCompatibility(&features) + mockedFeatures = features + expected = utils.CloneProtoMsg(&features) + requireFeatures(t, clock, *expected, handler.GetClusterFeatures) + + // test updating entitlements + features = proto.Features{ + Kubernetes: true, + Entitlements: map[string]*proto.EntitlementInfo{ + string(entitlements.ExternalAuditStorage): {Enabled: true}, + string(entitlements.AccessLists): {Enabled: true}, + string(entitlements.AccessMonitoring): {Enabled: true}, + string(entitlements.App): {Enabled: true}, + string(entitlements.CloudAuditLogRetention): {Enabled: true}, + }, + AccessRequests: &proto.AccessRequestsFeature{}, + } + entitlements.SupportEntitlementsCompatibility(&features) + mockedFeatures = features + + expected = &proto.Features{ + Kubernetes: true, + Entitlements: map[string]*proto.EntitlementInfo{ + string(entitlements.ExternalAuditStorage): {Enabled: true}, + string(entitlements.AccessLists): {Enabled: true}, + string(entitlements.AccessMonitoring): {Enabled: true}, + string(entitlements.App): {Enabled: true}, + string(entitlements.CloudAuditLogRetention): {Enabled: true}, + }, + AccessRequests: &proto.AccessRequestsFeature{}, + } + entitlements.SupportEntitlementsCompatibility(expected) + requireFeatures(t, clock, *expected, handler.GetClusterFeatures) + + // stop watcher and ensure it stops updating features + handler.stopFeaturesWatcher() + features = proto.Features{ + Kubernetes: !features.Kubernetes, + App: !features.App, + DB: true, + Entitlements: map[string]*proto.EntitlementInfo{}, + AccessRequests: &proto.AccessRequestsFeature{}, + } + entitlements.SupportEntitlementsCompatibility(&features) + mockedFeatures = features + expected = utils.CloneProtoMsg(&features) + // assert the handler never get these last features as the watcher is stopped + neverFeatures(t, clock, *expected, handler.GetClusterFeatures) +} + +// requireFeatures is a helper function that advances the clock, then +// calls `getFeatures` every 100ms for up to 1 second, until it +// returns the expected result (`want`). +func requireFeatures(t *testing.T, fakeClock clockwork.FakeClock, want proto.Features, getFeatures func() proto.Features) { + t.Helper() + + // Advance the clock so the service fetch and stores features + fakeClock.Advance(1 * time.Second) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + diff := cmp.Diff(want, getFeatures()) + if !assert.Empty(c, diff) { + t.Logf("Feature diff (-want +got):\n%s", diff) + } + }, 1*time.Second, time.Millisecond*100) +} + +// neverFeatures is a helper function that advances the clock, then +// calls `getFeatures` every 100ms for up to 1 second. If at some point `getFeatures` +// returns `doNotWant`, the test fails. +func neverFeatures(t *testing.T, fakeClock clockwork.FakeClock, doNotWant proto.Features, getFeatures func() proto.Features) { + t.Helper() + + fakeClock.Advance(1 * time.Second) + require.Never(t, func() bool { + return cmp.Diff(doNotWant, getFeatures()) == "" + }, 1*time.Second, time.Millisecond*100) +} From 98b3dc1b88a4c037fe34a38797c6770d8326090d Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Thu, 29 Aug 2024 08:46:25 -0300 Subject: [PATCH 03/28] Update godocs, fix typos, rename SupportEntitlementsCompatibility to BackfillFeatures --- entitlements/entitlements.go | 4 ++-- lib/service/connect.go | 2 +- lib/web/apiserver.go | 9 ++++----- lib/web/features.go | 4 +--- lib/web/features_test.go | 10 +++++----- 5 files changed, 13 insertions(+), 16 deletions(-) diff --git a/entitlements/entitlements.go b/entitlements/entitlements.go index 2809981afe29..4e5564317a01 100644 --- a/entitlements/entitlements.go +++ b/entitlements/entitlements.go @@ -59,11 +59,11 @@ var AllEntitlements = []EntitlementKind{ OktaUserSync, Policy, SAML, SessionLocks, UpsellAlert, UsageReporting, } -// supportEntitlementsCompatibility ensures entitlements are backwards compatible +// BackfillFeatures ensures entitlements are backwards compatible // If Entitlements are present, there are no changes // If Entitlements are not present, sets the entitlements fields to legacy field values // TODO(michellescripts) remove in v18 -func SupportEntitlementsCompatibility(features *proto.Features) { +func BackfillFeatures(features *proto.Features) { if len(features.Entitlements) > 0 { return } diff --git a/lib/service/connect.go b/lib/service/connect.go index 0e99a6f7f7a7..7da16e2b733c 100644 --- a/lib/service/connect.go +++ b/lib/service/connect.go @@ -1130,7 +1130,7 @@ func (process *TeleportProcess) getConnector(clientIdentity, serverIdentity *sta // Set cluster features and return successfully with a working connector. // TODO(michellescripts) remove clone & compatibility check in v18 cloned := apiutils.CloneProtoMsg(pingResponse.GetServerFeatures()) - entitlements.SupportEntitlementsCompatibility(cloned) + entitlements.BackfillFeatures(cloned) process.setClusterFeatures(cloned) process.setAuthSubjectiveAddr(pingResponse.RemoteAddr) process.logger.InfoContext(process.ExitContext(), "features loaded from auth server", "identity", clientIdentity.ID.Role, "features", pingResponse.GetServerFeatures()) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index bf507fd028d6..69ed883dce0e 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -21,6 +21,7 @@ package web import ( + "cmp" "compress/gzip" "context" "crypto/tls" @@ -126,7 +127,7 @@ const ( // IncludedResourceModeAll describes that all resources, requestable and available, should be returned. IncludedResourceModeAll = "all" // DefaultLicenseWatchInterval is the default time in which the license watcher - // should ping the auth server for new cluster features + // should ping the auth server for updated features DefaultLicenseWatchInterval = time.Minute * 5 ) @@ -177,7 +178,7 @@ type Handler struct { wsIODeadline time.Duration // featureWatcherStop is a channel used to emit a stop signal to the - // license watcher goroutine + // features watcher goroutine featureWatcherStop chan struct{} featureWatcherOnce sync.Once } @@ -342,9 +343,7 @@ func (c *Config) SetDefaults() { c.PresenceChecker = client.RunPresenceTask } - if c.LicenseWatchInterval == 0 { - c.LicenseWatchInterval = DefaultLicenseWatchInterval - } + c.LicenseWatchInterval = cmp.Or(c.LicenseWatchInterval, DefaultLicenseWatchInterval) } type APIHandler struct { diff --git a/lib/web/features.go b/lib/web/features.go index f91f80ed1666..7e3e983f3726 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -16,8 +16,6 @@ * along with this program. If not, see . */ -// Package web implements web proxy handler that provides -// web interface to view and connect to teleport nodes package web import ( @@ -31,7 +29,7 @@ func (h *Handler) SetClusterFeatures(features proto.Features) { h.Mutex.Lock() defer h.Mutex.Unlock() - entitlements.SupportEntitlementsCompatibility(&features) + entitlements.BackfillFeatures(&features) h.clusterFeatures = features } diff --git a/lib/web/features_test.go b/lib/web/features_test.go index af8ec8ded4bc..24b7e7f8901f 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -55,7 +55,7 @@ func TestFeaturesWatcher(t *testing.T) { Entitlements: map[string]*proto.EntitlementInfo{}, AccessRequests: &proto.AccessRequestsFeature{}, } - entitlements.SupportEntitlementsCompatibility(&features) + entitlements.BackfillFeatures(&features) expected := utils.CloneProtoMsg(&features) requireFeatures(t, clock, *expected, handler.GetClusterFeatures) @@ -65,7 +65,7 @@ func TestFeaturesWatcher(t *testing.T) { Entitlements: map[string]*proto.EntitlementInfo{}, AccessRequests: &proto.AccessRequestsFeature{}, } - entitlements.SupportEntitlementsCompatibility(&features) + entitlements.BackfillFeatures(&features) mockedFeatures = features expected = utils.CloneProtoMsg(&features) requireFeatures(t, clock, *expected, handler.GetClusterFeatures) @@ -82,7 +82,7 @@ func TestFeaturesWatcher(t *testing.T) { }, AccessRequests: &proto.AccessRequestsFeature{}, } - entitlements.SupportEntitlementsCompatibility(&features) + entitlements.BackfillFeatures(&features) mockedFeatures = features expected = &proto.Features{ @@ -96,7 +96,7 @@ func TestFeaturesWatcher(t *testing.T) { }, AccessRequests: &proto.AccessRequestsFeature{}, } - entitlements.SupportEntitlementsCompatibility(expected) + entitlements.BackfillFeatures(expected) requireFeatures(t, clock, *expected, handler.GetClusterFeatures) // stop watcher and ensure it stops updating features @@ -108,7 +108,7 @@ func TestFeaturesWatcher(t *testing.T) { Entitlements: map[string]*proto.EntitlementInfo{}, AccessRequests: &proto.AccessRequestsFeature{}, } - entitlements.SupportEntitlementsCompatibility(&features) + entitlements.BackfillFeatures(&features) mockedFeatures = features expected = utils.CloneProtoMsg(&features) // assert the handler never get these last features as the watcher is stopped From e2f31d32ec085843a66882148b9891937ee48ede Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Thu, 29 Aug 2024 08:55:59 -0300 Subject: [PATCH 04/28] Godocs; rename start and stop functions --- lib/web/apiserver.go | 4 ---- lib/web/features.go | 8 ++++++-- lib/web/features_test.go | 4 ++-- lib/web/server.go | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 69ed883dce0e..752eff15ea4c 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -159,10 +159,6 @@ type Handler struct { userConns atomic.Int32 // clusterFeatures contain flags for supported and unsupported features. - // Note: This field can become stale since it's only set on initial proxy - // startup. To get the latest feature flags you'll need to ping from the - // auth server. - // https://github.com/gravitational/teleport/issues/39161 clusterFeatures proto.Features // nodeWatcher is a services.NodeWatcher used by Assist to lookup nodes from diff --git a/lib/web/features.go b/lib/web/features.go index 7e3e983f3726..2779055c9268 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -25,6 +25,7 @@ import ( "github.com/gravitational/teleport/entitlements" ) +// SetClusterFeatures sets the flags for supported and unsupported features func (h *Handler) SetClusterFeatures(features proto.Features) { h.Mutex.Lock() defer h.Mutex.Unlock() @@ -33,6 +34,7 @@ func (h *Handler) SetClusterFeatures(features proto.Features) { h.clusterFeatures = features } +// GetClusterFeatures returns flags for supported and unsupported features. func (h *Handler) GetClusterFeatures() proto.Features { h.Mutex.Lock() defer h.Mutex.Unlock() @@ -40,7 +42,8 @@ func (h *Handler) GetClusterFeatures() proto.Features { return h.clusterFeatures } -func (h *Handler) startFeaturesWatcher() { +// startFeatureWatcher periodically pings the auth server and updates `clusterFeatures`. +func (h *Handler) startFeatureWatcher() { h.featureWatcherOnce.Do(func() { ticker := h.clock.NewTicker(h.cfg.LicenseWatchInterval) h.log.WithField("interval", h.cfg.LicenseWatchInterval).Info("Proxy handler features watcher has started") @@ -67,6 +70,7 @@ func (h *Handler) startFeaturesWatcher() { }) } -func (h *Handler) stopFeaturesWatcher() { +// stopFeatureWatcher stops the feature watcher +func (h *Handler) stopFeatureWatcher() { close(h.featureWatcherStop) } diff --git a/lib/web/features_test.go b/lib/web/features_test.go index 24b7e7f8901f..a2218a5cad4c 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -45,7 +45,7 @@ func TestFeaturesWatcher(t *testing.T) { // before running the watcher, features should match the value passed to the handler requireFeatures(t, clock, proto.Features{}, handler.GetClusterFeatures) - go handler.startFeaturesWatcher() + go handler.startFeatureWatcher() clock.BlockUntil(1) // after starting the watcher, handler.GetClusterFeatures should return @@ -100,7 +100,7 @@ func TestFeaturesWatcher(t *testing.T) { requireFeatures(t, clock, *expected, handler.GetClusterFeatures) // stop watcher and ensure it stops updating features - handler.stopFeaturesWatcher() + handler.stopFeatureWatcher() features = proto.Features{ Kubernetes: !features.Kubernetes, App: !features.App, diff --git a/lib/web/server.go b/lib/web/server.go index 2542821b9151..ac6e067eb86b 100644 --- a/lib/web/server.go +++ b/lib/web/server.go @@ -98,7 +98,7 @@ func (s *Server) Serve(l net.Listener) error { if closed { return trace.Errorf("serve called on previously closed server") } - go s.cfg.Handler.handler.startFeaturesWatcher() + go s.cfg.Handler.handler.startFeatureWatcher() return trace.Wrap(s.cfg.Server.Serve(l)) } @@ -132,7 +132,7 @@ func (s *Server) Shutdown(ctx context.Context) error { err = s.ln.Close() } s.mu.Unlock() - s.cfg.Handler.handler.stopFeaturesWatcher() + s.cfg.Handler.handler.stopFeatureWatcher() activeConnections := s.cfg.Handler.handler.userConns.Load() if activeConnections == 0 { From 0b30b7be86179d9e03a20d3643c54a74a3b11346 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Thu, 29 Aug 2024 08:58:31 -0300 Subject: [PATCH 05/28] Use `Feature` prefix in var names instead of `License` --- lib/web/apiserver.go | 12 ++++++------ lib/web/features.go | 4 ++-- lib/web/features_test.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 752eff15ea4c..98a9626e6d11 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -126,9 +126,9 @@ const ( IncludedResourceModeRequestable = "requestable" // IncludedResourceModeAll describes that all resources, requestable and available, should be returned. IncludedResourceModeAll = "all" - // DefaultLicenseWatchInterval is the default time in which the license watcher - // should ping the auth server for updated features - DefaultLicenseWatchInterval = time.Minute * 5 + // DefaultFeatureWatchInterval is the default time in which the feature watcher + // should ping the auth server to check for updated features + DefaultFeatureWatchInterval = time.Minute * 5 ) // healthCheckAppServerFunc defines a function used to perform a health check @@ -321,9 +321,9 @@ type Config struct { // IntegrationAppHandler handles App Access requests which use an Integration. IntegrationAppHandler app.ServerHandler - // LicenseWatchInterval is the interval between pings to the auth server + // FeatureWatchInterval is the interval between pings to the auth server // to fetch new cluster features - LicenseWatchInterval time.Duration + FeatureWatchInterval time.Duration } // SetDefaults ensures proper default values are set if @@ -339,7 +339,7 @@ func (c *Config) SetDefaults() { c.PresenceChecker = client.RunPresenceTask } - c.LicenseWatchInterval = cmp.Or(c.LicenseWatchInterval, DefaultLicenseWatchInterval) + c.FeatureWatchInterval = cmp.Or(c.FeatureWatchInterval, DefaultFeatureWatchInterval) } type APIHandler struct { diff --git a/lib/web/features.go b/lib/web/features.go index 2779055c9268..62f552b46a3c 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -45,8 +45,8 @@ func (h *Handler) GetClusterFeatures() proto.Features { // startFeatureWatcher periodically pings the auth server and updates `clusterFeatures`. func (h *Handler) startFeatureWatcher() { h.featureWatcherOnce.Do(func() { - ticker := h.clock.NewTicker(h.cfg.LicenseWatchInterval) - h.log.WithField("interval", h.cfg.LicenseWatchInterval).Info("Proxy handler features watcher has started") + ticker := h.clock.NewTicker(h.cfg.FeatureWatchInterval) + h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") ctx := context.Background() defer ticker.Stop() diff --git a/lib/web/features_test.go b/lib/web/features_test.go index a2218a5cad4c..95cccbbeb2bd 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -26,7 +26,7 @@ func TestFeaturesWatcher(t *testing.T) { handler := &Handler{ cfg: Config{ - LicenseWatchInterval: 100 * time.Millisecond, + FeatureWatchInterval: 100 * time.Millisecond, ProxyClient: &mockedPingTestProxy{ mockedPing: func(ctx context.Context) (proto.PingResponse, error) { return proto.PingResponse{ From 659007664e25847bc6399e8cffa751cb40ed530f Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Thu, 29 Aug 2024 09:56:35 -0300 Subject: [PATCH 06/28] Fix lint --- lib/service/connect_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/service/connect_test.go b/lib/service/connect_test.go index 5a2df2e136ef..bb31617040cf 100644 --- a/lib/service/connect_test.go +++ b/lib/service/connect_test.go @@ -276,7 +276,7 @@ func Test_supportEntitlementsCompatibility(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cloned := apiutils.CloneProtoMsg(tt.features) - supportEntitlementsCompatibility(cloned) + entitlements.BackfillFeatures(cloned) require.Equal(t, tt.expected, cloned.Entitlements) }) } From 77dc08327bb39a3144a09201dd2100e7928d4a5a Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Fri, 30 Aug 2024 16:49:28 -0300 Subject: [PATCH 07/28] Fix TestGetWebConfig_LegacyFeatureLimits --- lib/web/apiserver.go | 6 +++++- lib/web/apiserver_test.go | 38 +++++++++++++++++++++++++------------- lib/web/features.go | 7 +++++-- 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 98a9626e6d11..28a4842c6085 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -177,6 +177,9 @@ type Handler struct { // features watcher goroutine featureWatcherStop chan struct{} featureWatcherOnce sync.Once + // featureWatcherReady is a chan that the feature watcher closes + // to signal it is ready. Used in tests. + featureWatcherReady chan struct{} } // HandlerOption is a functional argument - an option that can be passed @@ -401,6 +404,7 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { tracer: cfg.TracerProvider.Tracer(teleport.ComponentWeb), wsIODeadline: wsIODeadline, featureWatcherStop: make(chan struct{}), + featureWatcherReady: make(chan struct{}), } if automaticUpgrades(cfg.ClusterFeatures) && h.cfg.AutomaticUpgradesChannels == nil { @@ -1122,6 +1126,7 @@ func (h *Handler) getUserContext(w http.ResponseWriter, r *http.Request, p httpr } desktopRecordingEnabled := recConfig.GetMode() != types.RecordOff + // TODO use cluster features instead of pinging auth server pingResp, err := clt.Ping(r.Context()) if err != nil { return nil, trace.Wrap(err) @@ -1752,7 +1757,6 @@ func setEntitlementsWithLegacyLogic(webCfg *webclient.WebConfig, clusterFeatures webCfg.Entitlements[string(entitlements.OIDC)] = webclient.EntitlementInfo{Enabled: clusterFeatures.GetOIDC()} webCfg.Entitlements[string(entitlements.Policy)] = webclient.EntitlementInfo{Enabled: clusterFeatures.GetPolicy() != nil && clusterFeatures.GetPolicy().Enabled} webCfg.Entitlements[string(entitlements.SAML)] = webclient.EntitlementInfo{Enabled: clusterFeatures.GetSAML()} - // set default Identity fields to legacy feature value webCfg.Entitlements[string(entitlements.AccessLists)] = webclient.EntitlementInfo{Enabled: true, Limit: clusterFeatures.GetAccessList().GetCreateLimit()} webCfg.Entitlements[string(entitlements.AccessMonitoring)] = webclient.EntitlementInfo{Enabled: clusterFeatures.GetAccessMonitoring().GetEnabled(), Limit: clusterFeatures.GetAccessMonitoring().GetMaxReportRangeLimit()} diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 65cc9958459e..855215ec50c9 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4744,6 +4744,7 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { ctx := context.Background() env := newWebPack(t, 1) + handler := env.proxies[0].handler.handler modules.SetTestModules(t, &modules.TestModules{ TestFeatures: modules.Features{ @@ -4758,6 +4759,10 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { }, }, }) + // start the feature watcher so the web config gets new features + go handler.startFeatureWatcher() + <-handler.featureWatcherReady // await until the watcher is ready to advance to clock + env.clock.Advance(DefaultFeatureWatchInterval * 2) expectedCfg := webclient.WebConfig{ Auth: webclient.WebConfigAuthSettings{ @@ -4805,20 +4810,27 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { PlayableDatabaseProtocols: player.SupportedDatabaseProtocols, } - // Make a request. - clt := env.proxies[0].newClient(t) - endpoint := clt.Endpoint("web", "config.js") - re, err := clt.Get(ctx, endpoint, nil) - require.NoError(t, err) - require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) + require.EventuallyWithT(t, func(c *assert.CollectT) { + t.Helper() + // Make a request. + clt := env.proxies[0].newClient(t) + endpoint := clt.Endpoint("web", "config.js") + re, err := clt.Get(ctx, endpoint, nil) + require.NoError(t, err) + require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) - // Response is type application/javascript, we need to strip off the variable name - // and the semicolon at the end, then we are left with json like object. - var cfg webclient.WebConfig - str := strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") - err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) - require.NoError(t, err) - require.Equal(t, expectedCfg, cfg) + // Response is type application/javascript, we need to strip off the variable name + // and the semicolon at the end, then we are left with json like object. + var cfg webclient.WebConfig + str := strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") + err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) + require.NoError(t, err) + diff := cmp.Diff(expectedCfg, cfg) + if assert.Empty(c, diff) { + t.Logf("Feature diff (-want +got):\n%s", diff) + } + + }, time.Second, time.Millisecond*50) } func TestCreatePrivilegeToken(t *testing.T) { diff --git a/lib/web/features.go b/lib/web/features.go index 62f552b46a3c..b4e912b9d454 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -49,18 +49,21 @@ func (h *Handler) startFeatureWatcher() { h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") ctx := context.Background() + // close ready channel to signal it started the main loop + close(h.featureWatcherReady) + defer ticker.Stop() for { select { case <-ticker.Chan(): h.log.Info("Pinging auth server for features") - f, err := h.cfg.ProxyClient.Ping(ctx) + pingResponse, err := h.GetProxyClient().Ping(ctx) if err != nil { h.log.WithError(err).Error("Auth server ping failed") continue } - h.SetClusterFeatures(*f.ServerFeatures) + h.SetClusterFeatures(*pingResponse.ServerFeatures) h.log.Debug("Done updating proxy features") case <-h.featureWatcherStop: h.log.Info("Feature service has stopped") From 74066c764cff8865e568f4120b900fa1cee7df1c Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Fri, 30 Aug 2024 17:06:17 -0300 Subject: [PATCH 08/28] Fix TestGetWebConfig_WithEntitlements --- lib/web/apiserver_test.go | 54 +++++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 855215ec50c9..069e7196d6f8 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4556,6 +4556,7 @@ func TestApplicationWebSessionsDeletedAfterLogout(t *testing.T) { func TestGetWebConfig_WithEntitlements(t *testing.T) { ctx := context.Background() env := newWebPack(t, 1) + handler := env.proxies[0].handler.handler // Set auth preference with passwordless. const MOTD = "Welcome to cluster, your activity will be recorded." @@ -4586,6 +4587,11 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { _, err = env.server.Auth().UpsertGithubConnector(ctx, github) require.NoError(t, err) + // start the feature watcher so the web config gets new features + go handler.startFeatureWatcher() + <-handler.featureWatcherReady // await until the watcher is ready + env.clock.Advance(DefaultFeatureWatchInterval * 2) + expectedCfg := webclient.WebConfig{ Auth: webclient.WebConfigAuthSettings{ SecondFactor: constants.SecondFactorOptional, @@ -4674,6 +4680,7 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { }, }, }) + env.clock.Advance(DefaultFeatureWatchInterval * 2) require.NoError(t, err) // This version is too high and MUST NOT be used @@ -4684,7 +4691,7 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { }, } require.NoError(t, channels.CheckAndSetDefaults()) - env.proxies[0].handler.handler.cfg.AutomaticUpgradesChannels = channels + handler.cfg.AutomaticUpgradesChannels = channels expectedCfg.IsCloud = true expectedCfg.IsUsageBasedBilling = true @@ -4700,14 +4707,20 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { expectedCfg.Entitlements[string(entitlements.JoinActiveSessions)] = webclient.EntitlementInfo{Enabled: false} expectedCfg.Entitlements[string(entitlements.K8s)] = webclient.EntitlementInfo{Enabled: false} - // request and verify enabled features are enabled. - re, err = clt.Get(ctx, endpoint, nil) - require.NoError(t, err) - require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) - str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") - err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) - require.NoError(t, err) - require.Equal(t, expectedCfg, cfg) + // request and verify enabled features are eventually enabled. + require.EventuallyWithT(t, func(c *assert.CollectT) { + re, err = clt.Get(ctx, endpoint, nil) + require.NoError(t, err) + require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) + str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") + err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) + require.NoError(t, err) + diff := cmp.Diff(expectedCfg, cfg) + if assert.Empty(c, diff) { + t.Logf("Feature diff (-want +got):\n%s", diff) + } + + }, time.Second, time.Millisecond*50) // use mock client to assert that if ping returns an error, we'll default to // cluster config @@ -4730,15 +4743,22 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { IsUsageBasedBilling: false, }, }) + env.clock.Advance(DefaultFeatureWatchInterval * 2) // request and verify again - re, err = clt.Get(ctx, endpoint, nil) - require.NoError(t, err) - require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) - str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") - err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) - require.NoError(t, err) - require.Equal(t, expectedCfg, cfg) + require.EventuallyWithT(t, func(c *assert.CollectT) { + re, err = clt.Get(ctx, endpoint, nil) + require.NoError(t, err) + require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) + str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") + err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) + require.NoError(t, err) + diff := cmp.Diff(expectedCfg, cfg) + if assert.Empty(c, diff) { + t.Logf("Feature diff (-want +got):\n%s", diff) + } + + }, time.Second, time.Millisecond*50) } func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { @@ -4761,7 +4781,7 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { }) // start the feature watcher so the web config gets new features go handler.startFeatureWatcher() - <-handler.featureWatcherReady // await until the watcher is ready to advance to clock + <-handler.featureWatcherReady // await until the watcher is ready env.clock.Advance(DefaultFeatureWatchInterval * 2) expectedCfg := webclient.WebConfig{ From 5c8233ed5b36254b6967829eee63093800ef789b Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Mon, 2 Sep 2024 09:50:37 -0300 Subject: [PATCH 09/28] Fix tests and lint --- lib/web/features.go | 6 ++++-- lib/web/features_test.go | 25 ++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/web/features.go b/lib/web/features.go index b4e912b9d454..beac3dccc9e6 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -1,6 +1,6 @@ /* * Teleport - * Copyright (C) 2023 Gravitational, Inc. + * Copyright (C) 2024 Gravitational, Inc. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as published by @@ -50,7 +50,9 @@ func (h *Handler) startFeatureWatcher() { ctx := context.Background() // close ready channel to signal it started the main loop - close(h.featureWatcherReady) + if h.featureWatcherReady != nil { + close(h.featureWatcherReady) + } defer ticker.Stop() for { diff --git a/lib/web/features_test.go b/lib/web/features_test.go index 95cccbbeb2bd..dc157316d533 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -1,3 +1,21 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + package web import ( @@ -7,13 +25,14 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/jonboulle/clockwork" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/entitlements" - "github.com/jonboulle/clockwork" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestFeaturesWatcher(t *testing.T) { From 05751aa740be03d08ff996bf662e3427eef50ec0 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Mon, 2 Sep 2024 11:20:59 -0300 Subject: [PATCH 10/28] Remove sync.Once --- lib/web/apiserver.go | 1 - lib/web/features.go | 48 +++++++++++++++++++++----------------------- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 28a4842c6085..936998e85154 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -176,7 +176,6 @@ type Handler struct { // featureWatcherStop is a channel used to emit a stop signal to the // features watcher goroutine featureWatcherStop chan struct{} - featureWatcherOnce sync.Once // featureWatcherReady is a chan that the feature watcher closes // to signal it is ready. Used in tests. featureWatcherReady chan struct{} diff --git a/lib/web/features.go b/lib/web/features.go index beac3dccc9e6..8c043fea1502 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -44,35 +44,33 @@ func (h *Handler) GetClusterFeatures() proto.Features { // startFeatureWatcher periodically pings the auth server and updates `clusterFeatures`. func (h *Handler) startFeatureWatcher() { - h.featureWatcherOnce.Do(func() { - ticker := h.clock.NewTicker(h.cfg.FeatureWatchInterval) - h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") - ctx := context.Background() + ticker := h.clock.NewTicker(h.cfg.FeatureWatchInterval) + h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") + ctx := context.Background() - // close ready channel to signal it started the main loop - if h.featureWatcherReady != nil { - close(h.featureWatcherReady) - } - - defer ticker.Stop() - for { - select { - case <-ticker.Chan(): - h.log.Info("Pinging auth server for features") - pingResponse, err := h.GetProxyClient().Ping(ctx) - if err != nil { - h.log.WithError(err).Error("Auth server ping failed") - continue - } + // close ready channel to signal it started the main loop + if h.featureWatcherReady != nil { + close(h.featureWatcherReady) + } - h.SetClusterFeatures(*pingResponse.ServerFeatures) - h.log.Debug("Done updating proxy features") - case <-h.featureWatcherStop: - h.log.Info("Feature service has stopped") - return + defer ticker.Stop() + for { + select { + case <-ticker.Chan(): + h.log.Info("Pinging auth server for features") + pingResponse, err := h.GetProxyClient().Ping(ctx) + if err != nil { + h.log.WithError(err).Error("Auth server ping failed") + continue } + + h.SetClusterFeatures(*pingResponse.ServerFeatures) + h.log.Debug("Done updating proxy features") + case <-h.featureWatcherStop: + h.log.Info("Feature service has stopped") + return } - }) + } } // stopFeatureWatcher stops the feature watcher From 239b1f9ad98899fe86eb52855a50cc019cd2076d Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Mon, 2 Sep 2024 11:29:18 -0300 Subject: [PATCH 11/28] Add jitter --- lib/service/service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/service/service.go b/lib/service/service.go index 86c728de539d..e05a5b9fed37 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -4664,6 +4664,7 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error { TracerProvider: process.TracingProvider, AutomaticUpgradesChannels: cfg.Proxy.AutomaticUpgradesChannels, IntegrationAppHandler: connectionsHandler, + FeatureWatchInterval: utils.HalfJitter(web.DefaultFeatureWatchInterval * 2), } webHandler, err := web.NewHandler(webConfig) if err != nil { From b6ec2b324a45065175cde8af446a369692fbf619 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Mon, 2 Sep 2024 11:46:54 -0300 Subject: [PATCH 12/28] Remove Ping call from getUserContext --- lib/web/apiserver.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 936998e85154..7328e45b434f 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -1125,18 +1125,12 @@ func (h *Handler) getUserContext(w http.ResponseWriter, r *http.Request, p httpr } desktopRecordingEnabled := recConfig.GetMode() != types.RecordOff - // TODO use cluster features instead of pinging auth server - pingResp, err := clt.Ping(r.Context()) - if err != nil { - return nil, trace.Wrap(err) - } - - features := pingResp.GetServerFeatures() - entitlement := modules.GetProtoEntitlement(features, entitlements.AccessMonitoring) + features := h.GetClusterFeatures() + entitlement := modules.GetProtoEntitlement(&features, entitlements.AccessMonitoring) // ensure entitlement is set & feature is configured accessMonitoringEnabled := entitlement.Enabled && features.GetAccessMonitoringConfigured() - userContext, err := ui.NewUserContext(user, accessChecker.Roles(), *pingResp.ServerFeatures, desktopRecordingEnabled, accessMonitoringEnabled) + userContext, err := ui.NewUserContext(user, accessChecker.Roles(), features, desktopRecordingEnabled, accessMonitoringEnabled) if err != nil { return nil, trace.Wrap(err) } From 663d99c833da4d0724bbd87a0f7cd6a7fd01d401 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 13:32:30 -0300 Subject: [PATCH 13/28] Move entitlements test to entitlements package --- entitlements/entitlements_test.go | 282 +++++++++++++++++++++++++++++ lib/service/connect_test.go | 283 ------------------------------ 2 files changed, 282 insertions(+), 283 deletions(-) create mode 100644 entitlements/entitlements_test.go delete mode 100644 lib/service/connect_test.go diff --git a/entitlements/entitlements_test.go b/entitlements/entitlements_test.go new file mode 100644 index 000000000000..7ac2f450589e --- /dev/null +++ b/entitlements/entitlements_test.go @@ -0,0 +1,282 @@ +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package entitlements + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/gravitational/teleport/api/client/proto" + apiutils "github.com/gravitational/teleport/api/utils" +) + +func TestBackfillFeatures(t *testing.T) { + tests := []struct { + name string + features *proto.Features + expected map[string]*proto.EntitlementInfo + }{ + { + name: "entitlements present; keeps entitlement values", + features: &proto.Features{ + DeviceTrust: nil, + AccessRequests: nil, + AccessList: nil, + AccessMonitoring: nil, + Policy: nil, + CustomTheme: "", + ProductType: 0, + SupportType: 0, + Kubernetes: false, + App: false, + DB: false, + OIDC: false, + SAML: false, + AccessControls: false, + AdvancedAccessWorkflows: false, + Cloud: false, + HSM: false, + Desktop: false, + RecoveryCodes: false, + Plugins: false, + AutomaticUpgrades: false, + IsUsageBased: false, + Assist: false, + FeatureHiding: false, + IdentityGovernance: false, + AccessGraph: false, + Questionnaire: false, + IsStripeManaged: false, + ExternalAuditStorage: false, + JoinActiveSessions: false, + MobileDeviceManagement: false, + AccessMonitoringConfigured: false, + Entitlements: map[string]*proto.EntitlementInfo{ + string(AccessLists): {Enabled: true, Limit: 111}, + string(AccessMonitoring): {Enabled: true, Limit: 2113}, + string(AccessRequests): {Enabled: true, Limit: 39}, + string(App): {Enabled: false}, + string(CloudAuditLogRetention): {Enabled: true}, + string(DB): {Enabled: true}, + string(Desktop): {Enabled: true}, + string(DeviceTrust): {Enabled: true, Limit: 103}, + string(ExternalAuditStorage): {Enabled: true}, + string(FeatureHiding): {Enabled: true}, + string(HSM): {Enabled: true}, + string(Identity): {Enabled: true}, + string(JoinActiveSessions): {Enabled: true}, + string(K8s): {Enabled: true}, + string(MobileDeviceManagement): {Enabled: true}, + string(OIDC): {Enabled: true}, + string(OktaSCIM): {Enabled: true}, + string(OktaUserSync): {Enabled: true}, + string(Policy): {Enabled: true}, + string(SAML): {Enabled: true}, + string(SessionLocks): {Enabled: true}, + string(UpsellAlert): {Enabled: true}, + string(UsageReporting): {Enabled: true}, + }, + }, + expected: map[string]*proto.EntitlementInfo{ + string(AccessLists): {Enabled: true, Limit: 111}, + string(AccessMonitoring): {Enabled: true, Limit: 2113}, + string(AccessRequests): {Enabled: true, Limit: 39}, + string(App): {Enabled: false}, + string(CloudAuditLogRetention): {Enabled: true}, + string(DB): {Enabled: true}, + string(Desktop): {Enabled: true}, + string(DeviceTrust): {Enabled: true, Limit: 103}, + string(ExternalAuditStorage): {Enabled: true}, + string(FeatureHiding): {Enabled: true}, + string(HSM): {Enabled: true}, + string(Identity): {Enabled: true}, + string(JoinActiveSessions): {Enabled: true}, + string(K8s): {Enabled: true}, + string(MobileDeviceManagement): {Enabled: true}, + string(OIDC): {Enabled: true}, + string(OktaSCIM): {Enabled: true}, + string(OktaUserSync): {Enabled: true}, + string(Policy): {Enabled: true}, + string(SAML): {Enabled: true}, + string(SessionLocks): {Enabled: true}, + string(UpsellAlert): {Enabled: true}, + string(UsageReporting): {Enabled: true}, + }, + }, + { + name: "entitlements not present; identity on - sets legacy fields & drops limits", + features: &proto.Features{ + DeviceTrust: &proto.DeviceTrustFeature{ + Enabled: true, + DevicesUsageLimit: 33, + }, + AccessRequests: &proto.AccessRequestsFeature{ + MonthlyRequestLimit: 22, + }, + AccessList: &proto.AccessListFeature{ + CreateLimit: 44, + }, + AccessMonitoring: &proto.AccessMonitoringFeature{ + Enabled: true, + MaxReportRangeLimit: 55, + }, + Policy: &proto.PolicyFeature{ + Enabled: true, + }, + CustomTheme: "", + ProductType: 0, + SupportType: 0, + Kubernetes: true, + App: true, + DB: true, + OIDC: true, + SAML: true, + AccessControls: true, + AdvancedAccessWorkflows: true, + Cloud: true, + HSM: true, + Desktop: true, + RecoveryCodes: true, + Plugins: true, + AutomaticUpgrades: true, + IsUsageBased: true, + Assist: true, + FeatureHiding: true, + IdentityGovernance: true, + AccessGraph: true, + Questionnaire: true, + IsStripeManaged: true, + ExternalAuditStorage: true, + JoinActiveSessions: true, + MobileDeviceManagement: true, + AccessMonitoringConfigured: true, + }, + expected: map[string]*proto.EntitlementInfo{ + string(AccessLists): {Enabled: true}, + string(AccessMonitoring): {Enabled: true}, + string(AccessRequests): {Enabled: true}, + string(App): {Enabled: true}, + string(DB): {Enabled: true}, + string(Desktop): {Enabled: true}, + string(DeviceTrust): {Enabled: true}, + string(ExternalAuditStorage): {Enabled: true}, + string(FeatureHiding): {Enabled: true}, + string(HSM): {Enabled: true}, + string(Identity): {Enabled: true}, + string(JoinActiveSessions): {Enabled: true}, + string(K8s): {Enabled: true}, + string(MobileDeviceManagement): {Enabled: true}, + string(OIDC): {Enabled: true}, + string(OktaSCIM): {Enabled: true}, + string(OktaUserSync): {Enabled: true}, + string(Policy): {Enabled: true}, + string(SAML): {Enabled: true}, + string(SessionLocks): {Enabled: true}, + // defaults, no legacy equivalent + string(UsageReporting): {Enabled: false}, + string(UpsellAlert): {Enabled: false}, + string(CloudAuditLogRetention): {Enabled: false}, + }, + }, + { + name: "entitlements not present; identity off - sets legacy fields", + features: &proto.Features{ + DeviceTrust: &proto.DeviceTrustFeature{ + Enabled: true, + DevicesUsageLimit: 33, + }, + AccessRequests: &proto.AccessRequestsFeature{ + MonthlyRequestLimit: 22, + }, + AccessList: &proto.AccessListFeature{ + CreateLimit: 44, + }, + AccessMonitoring: &proto.AccessMonitoringFeature{ + Enabled: true, + MaxReportRangeLimit: 55, + }, + Policy: &proto.PolicyFeature{ + Enabled: true, + }, + CustomTheme: "", + ProductType: 0, + SupportType: 0, + Kubernetes: true, + App: true, + DB: true, + OIDC: true, + SAML: true, + AccessControls: true, + AdvancedAccessWorkflows: true, + Cloud: true, + HSM: true, + Desktop: true, + RecoveryCodes: true, + Plugins: true, + AutomaticUpgrades: true, + IsUsageBased: true, + Assist: true, + FeatureHiding: true, + IdentityGovernance: false, + AccessGraph: true, + Questionnaire: true, + IsStripeManaged: true, + ExternalAuditStorage: true, + JoinActiveSessions: true, + MobileDeviceManagement: true, + AccessMonitoringConfigured: true, + }, + expected: map[string]*proto.EntitlementInfo{ + string(AccessLists): {Enabled: true, Limit: 44}, + string(AccessMonitoring): {Enabled: true, Limit: 55}, + string(AccessRequests): {Enabled: true, Limit: 22}, + string(DeviceTrust): {Enabled: true, Limit: 33}, + string(App): {Enabled: true}, + string(DB): {Enabled: true}, + string(Desktop): {Enabled: true}, + string(ExternalAuditStorage): {Enabled: true}, + string(FeatureHiding): {Enabled: true}, + string(HSM): {Enabled: true}, + string(JoinActiveSessions): {Enabled: true}, + string(K8s): {Enabled: true}, + string(MobileDeviceManagement): {Enabled: true}, + string(OIDC): {Enabled: true}, + string(Policy): {Enabled: true}, + string(SAML): {Enabled: true}, + + // defaults, no legacy equivalent + string(UsageReporting): {Enabled: false}, + string(UpsellAlert): {Enabled: false}, + string(CloudAuditLogRetention): {Enabled: false}, + // Identity off, fields false + string(Identity): {Enabled: false}, + string(SessionLocks): {Enabled: false}, + string(OktaSCIM): {Enabled: false}, + string(OktaUserSync): {Enabled: false}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cloned := apiutils.CloneProtoMsg(tt.features) + + BackfillFeatures(cloned) + require.Equal(t, tt.expected, cloned.Entitlements) + }) + } +} diff --git a/lib/service/connect_test.go b/lib/service/connect_test.go deleted file mode 100644 index bb31617040cf..000000000000 --- a/lib/service/connect_test.go +++ /dev/null @@ -1,283 +0,0 @@ -// Teleport -// Copyright (C) 2024 Gravitational, Inc. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -package service - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/gravitational/teleport/api/client/proto" - apiutils "github.com/gravitational/teleport/api/utils" - "github.com/gravitational/teleport/entitlements" -) - -func Test_supportEntitlementsCompatibility(t *testing.T) { - tests := []struct { - name string - features *proto.Features - expected map[string]*proto.EntitlementInfo - }{ - { - name: "entitlements present; keeps entitlement values", - features: &proto.Features{ - DeviceTrust: nil, - AccessRequests: nil, - AccessList: nil, - AccessMonitoring: nil, - Policy: nil, - CustomTheme: "", - ProductType: 0, - SupportType: 0, - Kubernetes: false, - App: false, - DB: false, - OIDC: false, - SAML: false, - AccessControls: false, - AdvancedAccessWorkflows: false, - Cloud: false, - HSM: false, - Desktop: false, - RecoveryCodes: false, - Plugins: false, - AutomaticUpgrades: false, - IsUsageBased: false, - Assist: false, - FeatureHiding: false, - IdentityGovernance: false, - AccessGraph: false, - Questionnaire: false, - IsStripeManaged: false, - ExternalAuditStorage: false, - JoinActiveSessions: false, - MobileDeviceManagement: false, - AccessMonitoringConfigured: false, - Entitlements: map[string]*proto.EntitlementInfo{ - string(entitlements.AccessLists): {Enabled: true, Limit: 111}, - string(entitlements.AccessMonitoring): {Enabled: true, Limit: 2113}, - string(entitlements.AccessRequests): {Enabled: true, Limit: 39}, - string(entitlements.App): {Enabled: false}, - string(entitlements.CloudAuditLogRetention): {Enabled: true}, - string(entitlements.DB): {Enabled: true}, - string(entitlements.Desktop): {Enabled: true}, - string(entitlements.DeviceTrust): {Enabled: true, Limit: 103}, - string(entitlements.ExternalAuditStorage): {Enabled: true}, - string(entitlements.FeatureHiding): {Enabled: true}, - string(entitlements.HSM): {Enabled: true}, - string(entitlements.Identity): {Enabled: true}, - string(entitlements.JoinActiveSessions): {Enabled: true}, - string(entitlements.K8s): {Enabled: true}, - string(entitlements.MobileDeviceManagement): {Enabled: true}, - string(entitlements.OIDC): {Enabled: true}, - string(entitlements.OktaSCIM): {Enabled: true}, - string(entitlements.OktaUserSync): {Enabled: true}, - string(entitlements.Policy): {Enabled: true}, - string(entitlements.SAML): {Enabled: true}, - string(entitlements.SessionLocks): {Enabled: true}, - string(entitlements.UpsellAlert): {Enabled: true}, - string(entitlements.UsageReporting): {Enabled: true}, - }, - }, - expected: map[string]*proto.EntitlementInfo{ - string(entitlements.AccessLists): {Enabled: true, Limit: 111}, - string(entitlements.AccessMonitoring): {Enabled: true, Limit: 2113}, - string(entitlements.AccessRequests): {Enabled: true, Limit: 39}, - string(entitlements.App): {Enabled: false}, - string(entitlements.CloudAuditLogRetention): {Enabled: true}, - string(entitlements.DB): {Enabled: true}, - string(entitlements.Desktop): {Enabled: true}, - string(entitlements.DeviceTrust): {Enabled: true, Limit: 103}, - string(entitlements.ExternalAuditStorage): {Enabled: true}, - string(entitlements.FeatureHiding): {Enabled: true}, - string(entitlements.HSM): {Enabled: true}, - string(entitlements.Identity): {Enabled: true}, - string(entitlements.JoinActiveSessions): {Enabled: true}, - string(entitlements.K8s): {Enabled: true}, - string(entitlements.MobileDeviceManagement): {Enabled: true}, - string(entitlements.OIDC): {Enabled: true}, - string(entitlements.OktaSCIM): {Enabled: true}, - string(entitlements.OktaUserSync): {Enabled: true}, - string(entitlements.Policy): {Enabled: true}, - string(entitlements.SAML): {Enabled: true}, - string(entitlements.SessionLocks): {Enabled: true}, - string(entitlements.UpsellAlert): {Enabled: true}, - string(entitlements.UsageReporting): {Enabled: true}, - }, - }, - { - name: "entitlements not present; identity on - sets legacy fields & drops limits", - features: &proto.Features{ - DeviceTrust: &proto.DeviceTrustFeature{ - Enabled: true, - DevicesUsageLimit: 33, - }, - AccessRequests: &proto.AccessRequestsFeature{ - MonthlyRequestLimit: 22, - }, - AccessList: &proto.AccessListFeature{ - CreateLimit: 44, - }, - AccessMonitoring: &proto.AccessMonitoringFeature{ - Enabled: true, - MaxReportRangeLimit: 55, - }, - Policy: &proto.PolicyFeature{ - Enabled: true, - }, - CustomTheme: "", - ProductType: 0, - SupportType: 0, - Kubernetes: true, - App: true, - DB: true, - OIDC: true, - SAML: true, - AccessControls: true, - AdvancedAccessWorkflows: true, - Cloud: true, - HSM: true, - Desktop: true, - RecoveryCodes: true, - Plugins: true, - AutomaticUpgrades: true, - IsUsageBased: true, - Assist: true, - FeatureHiding: true, - IdentityGovernance: true, - AccessGraph: true, - Questionnaire: true, - IsStripeManaged: true, - ExternalAuditStorage: true, - JoinActiveSessions: true, - MobileDeviceManagement: true, - AccessMonitoringConfigured: true, - }, - expected: map[string]*proto.EntitlementInfo{ - string(entitlements.AccessLists): {Enabled: true}, - string(entitlements.AccessMonitoring): {Enabled: true}, - string(entitlements.AccessRequests): {Enabled: true}, - string(entitlements.App): {Enabled: true}, - string(entitlements.DB): {Enabled: true}, - string(entitlements.Desktop): {Enabled: true}, - string(entitlements.DeviceTrust): {Enabled: true}, - string(entitlements.ExternalAuditStorage): {Enabled: true}, - string(entitlements.FeatureHiding): {Enabled: true}, - string(entitlements.HSM): {Enabled: true}, - string(entitlements.Identity): {Enabled: true}, - string(entitlements.JoinActiveSessions): {Enabled: true}, - string(entitlements.K8s): {Enabled: true}, - string(entitlements.MobileDeviceManagement): {Enabled: true}, - string(entitlements.OIDC): {Enabled: true}, - string(entitlements.OktaSCIM): {Enabled: true}, - string(entitlements.OktaUserSync): {Enabled: true}, - string(entitlements.Policy): {Enabled: true}, - string(entitlements.SAML): {Enabled: true}, - string(entitlements.SessionLocks): {Enabled: true}, - // defaults, no legacy equivalent - string(entitlements.UsageReporting): {Enabled: false}, - string(entitlements.UpsellAlert): {Enabled: false}, - string(entitlements.CloudAuditLogRetention): {Enabled: false}, - }, - }, - { - name: "entitlements not present; identity off - sets legacy fields", - features: &proto.Features{ - DeviceTrust: &proto.DeviceTrustFeature{ - Enabled: true, - DevicesUsageLimit: 33, - }, - AccessRequests: &proto.AccessRequestsFeature{ - MonthlyRequestLimit: 22, - }, - AccessList: &proto.AccessListFeature{ - CreateLimit: 44, - }, - AccessMonitoring: &proto.AccessMonitoringFeature{ - Enabled: true, - MaxReportRangeLimit: 55, - }, - Policy: &proto.PolicyFeature{ - Enabled: true, - }, - CustomTheme: "", - ProductType: 0, - SupportType: 0, - Kubernetes: true, - App: true, - DB: true, - OIDC: true, - SAML: true, - AccessControls: true, - AdvancedAccessWorkflows: true, - Cloud: true, - HSM: true, - Desktop: true, - RecoveryCodes: true, - Plugins: true, - AutomaticUpgrades: true, - IsUsageBased: true, - Assist: true, - FeatureHiding: true, - IdentityGovernance: false, - AccessGraph: true, - Questionnaire: true, - IsStripeManaged: true, - ExternalAuditStorage: true, - JoinActiveSessions: true, - MobileDeviceManagement: true, - AccessMonitoringConfigured: true, - }, - expected: map[string]*proto.EntitlementInfo{ - string(entitlements.AccessLists): {Enabled: true, Limit: 44}, - string(entitlements.AccessMonitoring): {Enabled: true, Limit: 55}, - string(entitlements.AccessRequests): {Enabled: true, Limit: 22}, - string(entitlements.DeviceTrust): {Enabled: true, Limit: 33}, - string(entitlements.App): {Enabled: true}, - string(entitlements.DB): {Enabled: true}, - string(entitlements.Desktop): {Enabled: true}, - string(entitlements.ExternalAuditStorage): {Enabled: true}, - string(entitlements.FeatureHiding): {Enabled: true}, - string(entitlements.HSM): {Enabled: true}, - string(entitlements.JoinActiveSessions): {Enabled: true}, - string(entitlements.K8s): {Enabled: true}, - string(entitlements.MobileDeviceManagement): {Enabled: true}, - string(entitlements.OIDC): {Enabled: true}, - string(entitlements.Policy): {Enabled: true}, - string(entitlements.SAML): {Enabled: true}, - - // defaults, no legacy equivalent - string(entitlements.UsageReporting): {Enabled: false}, - string(entitlements.UpsellAlert): {Enabled: false}, - string(entitlements.CloudAuditLogRetention): {Enabled: false}, - // Identity off, fields false - string(entitlements.Identity): {Enabled: false}, - string(entitlements.SessionLocks): {Enabled: false}, - string(entitlements.OktaSCIM): {Enabled: false}, - string(entitlements.OktaUserSync): {Enabled: false}, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cloned := apiutils.CloneProtoMsg(tt.features) - - entitlements.BackfillFeatures(cloned) - require.Equal(t, tt.expected, cloned.Entitlements) - }) - } -} From 2899bfa6759e54a868186789207273d339959026 Mon Sep 17 00:00:00 2001 From: matheus Date: Wed, 4 Sep 2024 13:34:44 -0300 Subject: [PATCH 14/28] Apply suggestions from code review: godoc and tests improvement. Co-authored-by: Zac Bergquist --- entitlements/entitlements.go | 6 +++--- lib/web/apiserver_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/entitlements/entitlements.go b/entitlements/entitlements.go index 4e5564317a01..bfb5879f3a2c 100644 --- a/entitlements/entitlements.go +++ b/entitlements/entitlements.go @@ -59,9 +59,9 @@ var AllEntitlements = []EntitlementKind{ OktaUserSync, Policy, SAML, SessionLocks, UpsellAlert, UsageReporting, } -// BackfillFeatures ensures entitlements are backwards compatible -// If Entitlements are present, there are no changes -// If Entitlements are not present, sets the entitlements fields to legacy field values +// BackfillFeatures ensures entitlements are backwards compatible. +// If Entitlements are present, there are no changes. +// If Entitlements are not present, it sets the entitlements based on legacy field values. // TODO(michellescripts) remove in v18 func BackfillFeatures(features *proto.Features) { if len(features.Entitlements) > 0 { diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 069e7196d6f8..af07823aca14 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4708,9 +4708,9 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { expectedCfg.Entitlements[string(entitlements.K8s)] = webclient.EntitlementInfo{Enabled: false} // request and verify enabled features are eventually enabled. - require.EventuallyWithT(t, func(c *assert.CollectT) { + require.EventuallyWithT(t, func(t *assert.CollectT) { re, err = clt.Get(ctx, endpoint, nil) - require.NoError(t, err) + assert.NoError(t, err) require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) From 7ab176048522a04b6bf8800c2b404023785a5de2 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 13:53:11 -0300 Subject: [PATCH 15/28] Improve tests --- lib/web/apiserver_test.go | 44 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index af07823aca14..7e196ce9e31b 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4708,19 +4708,19 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { expectedCfg.Entitlements[string(entitlements.K8s)] = webclient.EntitlementInfo{Enabled: false} // request and verify enabled features are eventually enabled. - require.EventuallyWithT(t, func(t *assert.CollectT) { - re, err = clt.Get(ctx, endpoint, nil) - assert.NoError(t, err) - require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) - str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") - err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) - require.NoError(t, err) + require.EventuallyWithT(t, func(c *assert.CollectT) { + re, err := clt.Get(ctx, endpoint, nil) + assert.NoError(c, err) + assert.True(c, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) + res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) + err = json.Unmarshal(res[:len(res)-1], &cfg) + assert.NoError(c, err) diff := cmp.Diff(expectedCfg, cfg) if assert.Empty(c, diff) { t.Logf("Feature diff (-want +got):\n%s", diff) } - }, time.Second, time.Millisecond*50) + }, time.Second*5, time.Millisecond*50) // use mock client to assert that if ping returns an error, we'll default to // cluster config @@ -4747,18 +4747,18 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { // request and verify again require.EventuallyWithT(t, func(c *assert.CollectT) { - re, err = clt.Get(ctx, endpoint, nil) - require.NoError(t, err) - require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) - str = strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") - err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) - require.NoError(t, err) + re, err := clt.Get(ctx, endpoint, nil) + assert.NoError(c, err) + assert.True(c, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) + res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) + err = json.Unmarshal(res[:len(res)-1], &cfg) + assert.NoError(c, err) diff := cmp.Diff(expectedCfg, cfg) if assert.Empty(c, diff) { t.Logf("Feature diff (-want +got):\n%s", diff) } - }, time.Second, time.Millisecond*50) + }, time.Second*5, time.Millisecond*50) } func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { @@ -4830,27 +4830,27 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { PlayableDatabaseProtocols: player.SupportedDatabaseProtocols, } + clt := env.proxies[0].newClient(t) require.EventuallyWithT(t, func(c *assert.CollectT) { t.Helper() // Make a request. - clt := env.proxies[0].newClient(t) endpoint := clt.Endpoint("web", "config.js") re, err := clt.Get(ctx, endpoint, nil) - require.NoError(t, err) - require.True(t, strings.HasPrefix(string(re.Bytes()), "var GRV_CONFIG")) + assert.NoError(c, err) + assert.True(c, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) // Response is type application/javascript, we need to strip off the variable name // and the semicolon at the end, then we are left with json like object. var cfg webclient.WebConfig - str := strings.ReplaceAll(string(re.Bytes()), "var GRV_CONFIG = ", "") - err = json.Unmarshal([]byte(str[:len(str)-1]), &cfg) - require.NoError(t, err) + res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) + err = json.Unmarshal(res[:len(res)-1], &cfg) + assert.NoError(c, err) diff := cmp.Diff(expectedCfg, cfg) if assert.Empty(c, diff) { t.Logf("Feature diff (-want +got):\n%s", diff) } - }, time.Second, time.Millisecond*50) + }, time.Second*5, time.Millisecond*50) } func TestCreatePrivilegeToken(t *testing.T) { From 690dd2771eea39cdd4c96309743b1ed767023849 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 14:09:41 -0300 Subject: [PATCH 16/28] Shadow `t` in EventuallyWithT closure to avoid mistakes --- lib/web/apiserver_test.go | 38 +++++++++++++++----------------------- lib/web/features_test.go | 8 +++----- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 7e196ce9e31b..8775f3bc98aa 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4708,17 +4708,15 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { expectedCfg.Entitlements[string(entitlements.K8s)] = webclient.EntitlementInfo{Enabled: false} // request and verify enabled features are eventually enabled. - require.EventuallyWithT(t, func(c *assert.CollectT) { + require.EventuallyWithT(t, func(t *assert.CollectT) { re, err := clt.Get(ctx, endpoint, nil) - assert.NoError(c, err) - assert.True(c, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) + assert.NoError(t, err) + assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) - assert.NoError(c, err) + assert.NoError(t, err) diff := cmp.Diff(expectedCfg, cfg) - if assert.Empty(c, diff) { - t.Logf("Feature diff (-want +got):\n%s", diff) - } + assert.Empty(t, diff) }, time.Second*5, time.Millisecond*50) @@ -4746,17 +4744,15 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { env.clock.Advance(DefaultFeatureWatchInterval * 2) // request and verify again - require.EventuallyWithT(t, func(c *assert.CollectT) { + require.EventuallyWithT(t, func(t *assert.CollectT) { re, err := clt.Get(ctx, endpoint, nil) - assert.NoError(c, err) - assert.True(c, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) + assert.NoError(t, err) + assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) - assert.NoError(c, err) + assert.NoError(t, err) diff := cmp.Diff(expectedCfg, cfg) - if assert.Empty(c, diff) { - t.Logf("Feature diff (-want +got):\n%s", diff) - } + assert.Empty(t, diff) }, time.Second*5, time.Millisecond*50) } @@ -4831,25 +4827,21 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { } clt := env.proxies[0].newClient(t) - require.EventuallyWithT(t, func(c *assert.CollectT) { - t.Helper() + require.EventuallyWithT(t, func(t *assert.CollectT) { // Make a request. endpoint := clt.Endpoint("web", "config.js") re, err := clt.Get(ctx, endpoint, nil) - assert.NoError(c, err) - assert.True(c, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) + assert.NoError(t, err) + assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) // Response is type application/javascript, we need to strip off the variable name // and the semicolon at the end, then we are left with json like object. var cfg webclient.WebConfig res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) - assert.NoError(c, err) + assert.NoError(t, err) diff := cmp.Diff(expectedCfg, cfg) - if assert.Empty(c, diff) { - t.Logf("Feature diff (-want +got):\n%s", diff) - } - + assert.Empty(t, diff) }, time.Second*5, time.Millisecond*50) } diff --git a/lib/web/features_test.go b/lib/web/features_test.go index dc157316d533..64408fc11bb3 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -143,12 +143,10 @@ func requireFeatures(t *testing.T, fakeClock clockwork.FakeClock, want proto.Fea // Advance the clock so the service fetch and stores features fakeClock.Advance(1 * time.Second) - require.EventuallyWithT(t, func(c *assert.CollectT) { + require.EventuallyWithT(t, func(t *assert.CollectT) { diff := cmp.Diff(want, getFeatures()) - if !assert.Empty(c, diff) { - t.Logf("Feature diff (-want +got):\n%s", diff) - } - }, 1*time.Second, time.Millisecond*100) + assert.Empty(t, diff) + }, 5*time.Second, time.Millisecond*100) } // neverFeatures is a helper function that advances the clock, then From 5d3b39e7624aa6c5d8102f377167bb06f7d8ebc2 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 14:14:30 -0300 Subject: [PATCH 17/28] Improve startFeatureWatcher godoc --- lib/web/features.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/web/features.go b/lib/web/features.go index 8c043fea1502..0dc897cfc76a 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -43,6 +43,8 @@ func (h *Handler) GetClusterFeatures() proto.Features { } // startFeatureWatcher periodically pings the auth server and updates `clusterFeatures`. +// Must be called only once per `handler`, otherwise it may close an already closed channel +// which will cause a panic. func (h *Handler) startFeatureWatcher() { ticker := h.clock.NewTicker(h.cfg.FeatureWatchInterval) h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") From 53c097d11cc501e536c18ab986a5aed2cd14c953 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 14:17:14 -0300 Subject: [PATCH 18/28] Log features on update --- lib/web/features.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/features.go b/lib/web/features.go index 0dc897cfc76a..13ea734dd840 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -67,7 +67,7 @@ func (h *Handler) startFeatureWatcher() { } h.SetClusterFeatures(*pingResponse.ServerFeatures) - h.log.Debug("Done updating proxy features") + h.log.WithField("features", pingResponse.ServerFeatures).Debug("Done updating proxy features") case <-h.featureWatcherStop: h.log.Info("Feature service has stopped") return From 96997d868a11f2ba42b6a7f27a37f0dea85f4cb6 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 14:17:40 -0300 Subject: [PATCH 19/28] Log features on update --- lib/web/features.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/features.go b/lib/web/features.go index 13ea734dd840..44485c99d189 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -67,7 +67,7 @@ func (h *Handler) startFeatureWatcher() { } h.SetClusterFeatures(*pingResponse.ServerFeatures) - h.log.WithField("features", pingResponse.ServerFeatures).Debug("Done updating proxy features") + h.log.WithField("features", pingResponse.ServerFeatures).Info("Done updating proxy features") case <-h.featureWatcherStop: h.log.Info("Feature service has stopped") return From bcf8cb7ab75ab07292739f1df850991701bba3e6 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 4 Sep 2024 14:47:29 -0300 Subject: [PATCH 20/28] Avoid race condition on test --- lib/web/features_test.go | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/lib/web/features_test.go b/lib/web/features_test.go index 64408fc11bb3..55f5a47fc097 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -33,26 +33,39 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/entitlements" + "github.com/gravitational/teleport/lib/auth/authclient" ) +// mockedPingTestProxy is a test proxy with a mocked Ping method +// that returns the internal features +type mockedFeatureGetter struct { + authclient.ClientI + features proto.Features +} + +func (m *mockedFeatureGetter) Ping(ctx context.Context) (proto.PingResponse, error) { + return proto.PingResponse{ + ServerFeatures: utils.CloneProtoMsg(&m.features), + }, nil +} + +func (m *mockedFeatureGetter) setFeatures(f proto.Features) { + m.features = f +} + func TestFeaturesWatcher(t *testing.T) { clock := clockwork.NewFakeClock() - mockedFeatures := proto.Features{ + + mockClient := &mockedFeatureGetter{features: proto.Features{ Kubernetes: true, Entitlements: map[string]*proto.EntitlementInfo{}, AccessRequests: &proto.AccessRequestsFeature{}, - } + }} handler := &Handler{ cfg: Config{ FeatureWatchInterval: 100 * time.Millisecond, - ProxyClient: &mockedPingTestProxy{ - mockedPing: func(ctx context.Context) (proto.PingResponse, error) { - return proto.PingResponse{ - ServerFeatures: &mockedFeatures, - }, nil - }, - }, + ProxyClient: mockClient, }, clock: clock, clusterFeatures: proto.Features{}, @@ -85,7 +98,7 @@ func TestFeaturesWatcher(t *testing.T) { AccessRequests: &proto.AccessRequestsFeature{}, } entitlements.BackfillFeatures(&features) - mockedFeatures = features + mockClient.setFeatures(features) expected = utils.CloneProtoMsg(&features) requireFeatures(t, clock, *expected, handler.GetClusterFeatures) @@ -102,7 +115,7 @@ func TestFeaturesWatcher(t *testing.T) { AccessRequests: &proto.AccessRequestsFeature{}, } entitlements.BackfillFeatures(&features) - mockedFeatures = features + mockClient.setFeatures(features) expected = &proto.Features{ Kubernetes: true, @@ -128,7 +141,7 @@ func TestFeaturesWatcher(t *testing.T) { AccessRequests: &proto.AccessRequestsFeature{}, } entitlements.BackfillFeatures(&features) - mockedFeatures = features + mockClient.setFeatures(features) expected = utils.CloneProtoMsg(&features) // assert the handler never get these last features as the watcher is stopped neverFeatures(t, clock, *expected, handler.GetClusterFeatures) From e6abb87cf44cd40c885648c848b1e81a0d13e4b8 Mon Sep 17 00:00:00 2001 From: matheus Date: Thu, 12 Sep 2024 13:57:35 -0300 Subject: [PATCH 21/28] Improve TODO comment Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> --- entitlements/entitlements.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/entitlements/entitlements.go b/entitlements/entitlements.go index bfb5879f3a2c..6e485d2494ce 100644 --- a/entitlements/entitlements.go +++ b/entitlements/entitlements.go @@ -62,7 +62,7 @@ var AllEntitlements = []EntitlementKind{ // BackfillFeatures ensures entitlements are backwards compatible. // If Entitlements are present, there are no changes. // If Entitlements are not present, it sets the entitlements based on legacy field values. -// TODO(michellescripts) remove in v18 +// TODO(michellescripts) DELETE IN 18.0.0 func BackfillFeatures(features *proto.Features) { if len(features.Entitlements) > 0 { return From e27f758a1c15ccf99763382552d55e27f5aa2845 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Thu, 12 Sep 2024 16:23:20 -0300 Subject: [PATCH 22/28] Use handler config context --- lib/web/apiserver.go | 4 ---- lib/web/features.go | 11 ++--------- lib/web/features_test.go | 17 +++++++++-------- lib/web/server.go | 1 - 4 files changed, 11 insertions(+), 22 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index f42eb3965bc2..25e09d5447b2 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -173,9 +173,6 @@ type Handler struct { // open. wsIODeadline time.Duration - // featureWatcherStop is a channel used to emit a stop signal to the - // features watcher goroutine - featureWatcherStop chan struct{} // featureWatcherReady is a chan that the feature watcher closes // to signal it is ready. Used in tests. featureWatcherReady chan struct{} @@ -469,7 +466,6 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { healthCheckAppServer: cfg.HealthCheckAppServer, tracer: cfg.TracerProvider.Tracer(teleport.ComponentWeb), wsIODeadline: wsIODeadline, - featureWatcherStop: make(chan struct{}), featureWatcherReady: make(chan struct{}), } diff --git a/lib/web/features.go b/lib/web/features.go index 44485c99d189..395838b21158 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -19,8 +19,6 @@ package web import ( - "context" - "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/entitlements" ) @@ -48,7 +46,7 @@ func (h *Handler) GetClusterFeatures() proto.Features { func (h *Handler) startFeatureWatcher() { ticker := h.clock.NewTicker(h.cfg.FeatureWatchInterval) h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") - ctx := context.Background() + ctx := h.cfg.Context // close ready channel to signal it started the main loop if h.featureWatcherReady != nil { @@ -68,14 +66,9 @@ func (h *Handler) startFeatureWatcher() { h.SetClusterFeatures(*pingResponse.ServerFeatures) h.log.WithField("features", pingResponse.ServerFeatures).Info("Done updating proxy features") - case <-h.featureWatcherStop: + case <-ctx.Done(): h.log.Info("Feature service has stopped") return } } } - -// stopFeatureWatcher stops the feature watcher -func (h *Handler) stopFeatureWatcher() { - close(h.featureWatcherStop) -} diff --git a/lib/web/features_test.go b/lib/web/features_test.go index 55f5a47fc097..3798e819b46e 100644 --- a/lib/web/features_test.go +++ b/lib/web/features_test.go @@ -62,16 +62,17 @@ func TestFeaturesWatcher(t *testing.T) { AccessRequests: &proto.AccessRequestsFeature{}, }} + ctx, cancel := context.WithCancel(context.Background()) handler := &Handler{ cfg: Config{ FeatureWatchInterval: 100 * time.Millisecond, ProxyClient: mockClient, + Context: ctx, }, - clock: clock, - clusterFeatures: proto.Features{}, - featureWatcherStop: make(chan struct{}), - log: newPackageLogger(), - logger: slog.Default().With(teleport.ComponentKey, teleport.ComponentWeb), + clock: clock, + clusterFeatures: proto.Features{}, + log: newPackageLogger(), + logger: slog.Default().With(teleport.ComponentKey, teleport.ComponentWeb), } // before running the watcher, features should match the value passed to the handler @@ -132,7 +133,7 @@ func TestFeaturesWatcher(t *testing.T) { requireFeatures(t, clock, *expected, handler.GetClusterFeatures) // stop watcher and ensure it stops updating features - handler.stopFeatureWatcher() + cancel() features = proto.Features{ Kubernetes: !features.Kubernetes, App: !features.App, @@ -142,9 +143,9 @@ func TestFeaturesWatcher(t *testing.T) { } entitlements.BackfillFeatures(&features) mockClient.setFeatures(features) - expected = utils.CloneProtoMsg(&features) + notExpected := utils.CloneProtoMsg(&features) // assert the handler never get these last features as the watcher is stopped - neverFeatures(t, clock, *expected, handler.GetClusterFeatures) + neverFeatures(t, clock, *notExpected, handler.GetClusterFeatures) } // requireFeatures is a helper function that advances the clock, then diff --git a/lib/web/server.go b/lib/web/server.go index ac6e067eb86b..37e048cd9ed9 100644 --- a/lib/web/server.go +++ b/lib/web/server.go @@ -132,7 +132,6 @@ func (s *Server) Shutdown(ctx context.Context) error { err = s.ln.Close() } s.mu.Unlock() - s.cfg.Handler.handler.stopFeatureWatcher() activeConnections := s.cfg.Handler.handler.userConns.Load() if activeConnections == 0 { From f2059140edb191489423dd26c962094f053934ec Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 25 Sep 2024 10:08:44 -0300 Subject: [PATCH 23/28] Start feature watcher in NewHandler --- lib/web/apiserver.go | 2 ++ lib/web/apiserver_test.go | 2 -- lib/web/server.go | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 7ad2ea0d30bf..7b72fc9b77b0 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -693,6 +693,8 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { } } + go h.startFeatureWatcher() + return &APIHandler{ handler: h, appHandler: appHandler, diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 92b4d8513f60..448b3e8ce3ab 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4593,7 +4593,6 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { require.NoError(t, err) // start the feature watcher so the web config gets new features - go handler.startFeatureWatcher() <-handler.featureWatcherReady // await until the watcher is ready env.clock.Advance(DefaultFeatureWatchInterval * 2) @@ -4781,7 +4780,6 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { }, }) // start the feature watcher so the web config gets new features - go handler.startFeatureWatcher() <-handler.featureWatcherReady // await until the watcher is ready env.clock.Advance(DefaultFeatureWatchInterval * 2) diff --git a/lib/web/server.go b/lib/web/server.go index 37e048cd9ed9..0af94831d30e 100644 --- a/lib/web/server.go +++ b/lib/web/server.go @@ -98,7 +98,6 @@ func (s *Server) Serve(l net.Listener) error { if closed { return trace.Errorf("serve called on previously closed server") } - go s.cfg.Handler.handler.startFeatureWatcher() return trace.Wrap(s.cfg.Server.Serve(l)) } From 53c2f7955f48308dff078956b0399e2ee5f96dc9 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 25 Sep 2024 10:48:31 -0300 Subject: [PATCH 24/28] Improve startFeatureWatcher godocs --- lib/web/features.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/web/features.go b/lib/web/features.go index 395838b21158..755e79858cae 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -43,6 +43,8 @@ func (h *Handler) GetClusterFeatures() proto.Features { // startFeatureWatcher periodically pings the auth server and updates `clusterFeatures`. // Must be called only once per `handler`, otherwise it may close an already closed channel // which will cause a panic. +// The watcher doesn't ping the auth server immediately upon start because features are +// already set by the config object in `NewHandler`. func (h *Handler) startFeatureWatcher() { ticker := h.clock.NewTicker(h.cfg.FeatureWatchInterval) h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") From fddde0c55efeb5ecd1d9e4d227a2acb3872a722a Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 25 Sep 2024 13:00:00 -0300 Subject: [PATCH 25/28] Add TODO to unexport SetClusterFeatures --- lib/web/features.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/web/features.go b/lib/web/features.go index 755e79858cae..066c2e3a09bc 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -23,7 +23,9 @@ import ( "github.com/gravitational/teleport/entitlements" ) -// SetClusterFeatures sets the flags for supported and unsupported features +// SetClusterFeatures sets the flags for supported and unsupported features. +// TODO(mcbattirola): make method unexported, fix tests using it to set +// test modules instead. func (h *Handler) SetClusterFeatures(features proto.Features) { h.Mutex.Lock() defer h.Mutex.Unlock() From 837b4125695c44a5dc773bb5a6bc3a30769b27db Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Wed, 25 Sep 2024 13:05:37 -0300 Subject: [PATCH 26/28] Remove featureWatcherReady --- lib/web/apiserver.go | 5 ----- lib/web/apiserver_test.go | 3 --- lib/web/features.go | 5 ----- 3 files changed, 13 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 7b72fc9b77b0..921abf023213 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -171,10 +171,6 @@ type Handler struct { // an authenticated websocket so unauthenticated sockets dont get left // open. wsIODeadline time.Duration - - // featureWatcherReady is a chan that the feature watcher closes - // to signal it is ready. Used in tests. - featureWatcherReady chan struct{} } // HandlerOption is a functional argument - an option that can be passed @@ -465,7 +461,6 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { healthCheckAppServer: cfg.HealthCheckAppServer, tracer: cfg.TracerProvider.Tracer(teleport.ComponentWeb), wsIODeadline: wsIODeadline, - featureWatcherReady: make(chan struct{}), } if automaticUpgrades(cfg.ClusterFeatures) && h.cfg.AutomaticUpgradesChannels == nil { diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 448b3e8ce3ab..3c7ba6ea6e6c 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4593,7 +4593,6 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { require.NoError(t, err) // start the feature watcher so the web config gets new features - <-handler.featureWatcherReady // await until the watcher is ready env.clock.Advance(DefaultFeatureWatchInterval * 2) expectedCfg := webclient.WebConfig{ @@ -4764,7 +4763,6 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { ctx := context.Background() env := newWebPack(t, 1) - handler := env.proxies[0].handler.handler modules.SetTestModules(t, &modules.TestModules{ TestFeatures: modules.Features{ @@ -4780,7 +4778,6 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { }, }) // start the feature watcher so the web config gets new features - <-handler.featureWatcherReady // await until the watcher is ready env.clock.Advance(DefaultFeatureWatchInterval * 2) expectedCfg := webclient.WebConfig{ diff --git a/lib/web/features.go b/lib/web/features.go index 066c2e3a09bc..29798851aa7c 100644 --- a/lib/web/features.go +++ b/lib/web/features.go @@ -52,11 +52,6 @@ func (h *Handler) startFeatureWatcher() { h.log.WithField("interval", h.cfg.FeatureWatchInterval).Info("Proxy handler features watcher has started") ctx := h.cfg.Context - // close ready channel to signal it started the main loop - if h.featureWatcherReady != nil { - close(h.featureWatcherReady) - } - defer ticker.Stop() for { select { From 85c4cae21e0e5e227f4e85be26ba5ed818cb7664 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Mon, 30 Sep 2024 15:14:48 -0300 Subject: [PATCH 27/28] Use require in require.EventuallyWithT in cases where an error is not expected --- lib/web/apiserver_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 3c7ba6ea6e6c..23b3bf618665 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4713,7 +4713,7 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { // request and verify enabled features are eventually enabled. require.EventuallyWithT(t, func(t *assert.CollectT) { re, err := clt.Get(ctx, endpoint, nil) - assert.NoError(t, err) + require.NoError(t, err) assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) @@ -4749,7 +4749,7 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { // request and verify again require.EventuallyWithT(t, func(t *assert.CollectT) { re, err := clt.Get(ctx, endpoint, nil) - assert.NoError(t, err) + require.NoError(t, err) assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) @@ -4831,7 +4831,7 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { // Make a request. endpoint := clt.Endpoint("web", "config.js") re, err := clt.Get(ctx, endpoint, nil) - assert.NoError(t, err) + require.NoError(t, err) assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) // Response is type application/javascript, we need to strip off the variable name From 171deacfb5cd5f298815d61f3998f785bcad03e1 Mon Sep 17 00:00:00 2001 From: mcbattirola Date: Mon, 30 Sep 2024 15:50:40 -0300 Subject: [PATCH 28/28] Use return of assert.NoError` to return early on require.EventuallyWithT --- lib/web/apiserver_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 23b3bf618665..6f6803d0e9a9 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -4713,7 +4713,9 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { // request and verify enabled features are eventually enabled. require.EventuallyWithT(t, func(t *assert.CollectT) { re, err := clt.Get(ctx, endpoint, nil) - require.NoError(t, err) + if !assert.NoError(t, err) { + return + } assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) @@ -4749,7 +4751,9 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) { // request and verify again require.EventuallyWithT(t, func(t *assert.CollectT) { re, err := clt.Get(ctx, endpoint, nil) - require.NoError(t, err) + if !assert.NoError(t, err) { + return + } assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) res := bytes.ReplaceAll(re.Bytes(), []byte("var GRV_CONFIG = "), []byte{}) err = json.Unmarshal(res[:len(res)-1], &cfg) @@ -4831,7 +4835,9 @@ func TestGetWebConfig_LegacyFeatureLimits(t *testing.T) { // Make a request. endpoint := clt.Endpoint("web", "config.js") re, err := clt.Get(ctx, endpoint, nil) - require.NoError(t, err) + if !assert.NoError(t, err) { + return + } assert.True(t, bytes.HasPrefix(re.Bytes(), []byte("var GRV_CONFIG"))) // Response is type application/javascript, we need to strip off the variable name