From 38c1f7697031f91ca8a12d175737f20adf9faa2b Mon Sep 17 00:00:00 2001 From: John Mazzitelli Date: Fri, 12 Jan 2024 13:15:25 -0500 Subject: [PATCH] use built-in SA token refresh functionality within client-go fixes: https://github.com/kiali/kiali/issues/6924 --- .../authentication/header_auth_controller.go | 2 +- .../authentication/openid_auth_controller.go | 2 +- business/istio_status.go | 2 +- handlers/authentication.go | 6 +- handlers/authentication_test.go | 1 + kubernetes/cache/cache.go | 67 ++----------------- kubernetes/cache/cache_test.go | 38 ----------- kubernetes/cache/kube_cache.go | 5 -- kubernetes/client.go | 7 +- kubernetes/client_factory.go | 60 +---------------- kubernetes/client_factory_test.go | 57 ---------------- kubernetes/token.go | 17 +++-- kubernetes/token_test.go | 6 +- prometheus/client.go | 2 +- status/discover.go | 3 +- status/versions.go | 2 +- 16 files changed, 40 insertions(+), 237 deletions(-) diff --git a/business/authentication/header_auth_controller.go b/business/authentication/header_auth_controller.go index a982a43a0c..4d7e15a037 100644 --- a/business/authentication/header_auth_controller.go +++ b/business/authentication/header_auth_controller.go @@ -72,7 +72,7 @@ func (c headerAuthController) Authenticate(r *http.Request, w http.ResponseWrite } } - kialiToken, err := kubernetes.GetKialiTokenForHomeCluster() + kialiToken, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { return nil, err } diff --git a/business/authentication/openid_auth_controller.go b/business/authentication/openid_auth_controller.go index 0fb5f2bda2..6263606cdf 100644 --- a/business/authentication/openid_auth_controller.go +++ b/business/authentication/openid_auth_controller.go @@ -247,7 +247,7 @@ func (c OpenIdAuthController) ValidateSession(r *http.Request, w http.ResponseWr // If RBAC is off, it's assumed that the kubernetes cluster will reject the OpenId token. // Instead, we use the Kiali token and this has the side effect that all users will share the // same privileges. - token, err = kubernetes.GetKialiTokenForHomeCluster() + token, _, err = kubernetes.GetKialiTokenForHomeCluster() if err != nil { return nil, fmt.Errorf("error reading the Kiali ServiceAccount token: %w", err) } diff --git a/business/istio_status.go b/business/istio_status.go index f81c6eb63f..f8fde3bdcd 100644 --- a/business/istio_status.go +++ b/business/istio_status.go @@ -270,7 +270,7 @@ func getAddonStatus(name string, enabled bool, isCore bool, auth *config.Auth, u } if auth.UseKialiToken { - token, err := kubernetes.GetKialiTokenForHomeCluster() + token, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { log.Errorf("Could not read the Kiali Service Account token: %v", err) } diff --git a/handlers/authentication.go b/handlers/authentication.go index b22b0d452e..cef5c8a4e4 100644 --- a/handlers/authentication.go +++ b/handlers/authentication.go @@ -35,7 +35,7 @@ type sessionInfo struct { func NewAuthenticationHandler() (AuthenticationHandler, error) { // Read token from the filesystem - saToken, err := kubernetes.GetKialiTokenForHomeCluster() + saToken, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { return AuthenticationHandler{}, err } @@ -62,7 +62,7 @@ func (aHandler AuthenticationHandler) Handle(next http.Handler) http.Handler { } case config.AuthStrategyAnonymous: log.Tracef("Access to the server endpoint is not secured with credentials - letting request come in. Url: [%s]", r.URL.String()) - token, err := kubernetes.GetKialiTokenForHomeCluster() + token, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { token = "" } @@ -133,7 +133,7 @@ func AuthenticationInfo(w http.ResponseWriter, r *http.Request) { switch conf.Auth.Strategy { case config.AuthStrategyOpenshift: - token, err := kubernetes.GetKialiTokenForHomeCluster() + token, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { RespondWithDetailedError(w, http.StatusInternalServerError, "Error obtaining Kiali SA token", err.Error()) return diff --git a/handlers/authentication_test.go b/handlers/authentication_test.go index facf5543f5..b0efe8d82a 100644 --- a/handlers/authentication_test.go +++ b/handlers/authentication_test.go @@ -252,6 +252,7 @@ func (r *rejectClient) GetProjects(labelSelector string) ([]osproject_v1.Project func mockK8s(t *testing.T, reject bool) { kubernetes.KialiTokenForHomeCluster = "notrealtoken" + kubernetes.KialiTokenFileForHomeCluster = "notrealtokenpath" k8s := kubetest.NewFakeK8sClient(&osproject_v1.Project{ObjectMeta: meta_v1.ObjectMeta{Name: "tutorial"}}) k8s.OpenShift = true var kubeClient kubernetes.ClientInterface = k8s diff --git a/kubernetes/cache/cache.go b/kubernetes/cache/cache.go index b15fe70f0a..773e374d4a 100644 --- a/kubernetes/cache/cache.go +++ b/kubernetes/cache/cache.go @@ -1,7 +1,6 @@ package cache import ( - "context" "errors" "fmt" "sync" @@ -58,12 +57,7 @@ type kialiCacheImpl struct { // TODO: Get rid of embedding. KubeCache - // Stops the background goroutines which refresh the cache's - // service account token and poll for istiod's proxy status. - cleanup func() clientFactory kubernetes.ClientFactory - // How often the cache will check for kiali SA client changes. - clientRefreshPollingPeriod time.Duration // Maps a cluster name to a KubeCache kubeCache map[string]KubeCache refreshDuration time.Duration @@ -82,14 +76,13 @@ type kialiCacheImpl struct { func NewKialiCache(clientFactory kubernetes.ClientFactory, cfg config.Config) (KialiCache, error) { kialiCacheImpl := kialiCacheImpl{ - clientFactory: clientFactory, - clientRefreshPollingPeriod: time.Duration(time.Second * 60), - kubeCache: make(map[string]KubeCache), - refreshDuration: time.Duration(cfg.KubernetesConfig.CacheDuration) * time.Second, - tokenNamespaces: make(map[string]namespaceCache), - tokenNamespaceDuration: time.Duration(cfg.KubernetesConfig.CacheTokenNamespaceDuration) * time.Second, - proxyStatusStore: store.New[*kubernetes.ProxyStatus](), - registryStatusStore: store.New[*kubernetes.RegistryStatus](), + clientFactory: clientFactory, + kubeCache: make(map[string]KubeCache), + refreshDuration: time.Duration(cfg.KubernetesConfig.CacheDuration) * time.Second, + tokenNamespaces: make(map[string]namespaceCache), + tokenNamespaceDuration: time.Duration(cfg.KubernetesConfig.CacheTokenNamespaceDuration) * time.Second, + proxyStatusStore: store.New[*kubernetes.ProxyStatus](), + registryStatusStore: store.New[*kubernetes.RegistryStatus](), } for cluster, client := range clientFactory.GetSAClients() { @@ -114,19 +107,6 @@ func NewKialiCache(clientFactory kubernetes.ClientFactory, cfg config.Config) (K return nil, errors.New("home cluster not configured in kiali cache") } - // Starting background goroutines to: - // 1. Refresh the cache's service account token - // These will stop when the context is cancelled. - // Starting goroutines after any errors are handled so as not to leak goroutines. - ctx, cancel := context.WithCancel(context.Background()) - - // Note that this only watches for changes to the home cluster's token since it is - // expected that the remote cluster tokens will not change. However, that assumption - // may be wrong and in the future the cache may want to watch for changes to all client tokens. - kialiCacheImpl.watchForClientChanges(ctx, clientFactory.GetSAHomeClusterClient().GetToken()) - - kialiCacheImpl.cleanup = cancel - return &kialiCacheImpl, nil } @@ -157,39 +137,6 @@ func (c *kialiCacheImpl) Stop() { }(kc) } wg.Wait() - - c.cleanup() -} - -// watchForClientChanges watches for changes to the cache's service account client -// and recreates the cache(s) when the client changes. The client is updated when -// the token for the client changes. -func (c *kialiCacheImpl) watchForClientChanges(ctx context.Context, token string) { - ticker := time.NewTicker(c.clientRefreshPollingPeriod) - go func() { - for { - select { - case <-ticker.C: - if c.clientFactory.GetSAHomeClusterClient().GetToken() != token { - log.Info("[Kiali Cache] Updating cache with new token") - - if err := c.KubeCache.UpdateClient(c.clientFactory.GetSAHomeClusterClient()); err != nil { - log.Errorf("[Kiali Cache] Error updating cache with new token. Err: %s", err) - // Try again on the next tick without updating the token. - continue - } - - token = c.clientFactory.GetSAHomeClusterClient().GetToken() - } else { - log.Debug("[Kiali Cache] Nothing to refresh") - } - case <-ctx.Done(): - log.Debug("[Kiali Cache] Stopping watching for service account token changes") - ticker.Stop() - return - } - } - }() } func (c *kialiCacheImpl) GetClusters() []kubernetes.Cluster { diff --git a/kubernetes/cache/cache_test.go b/kubernetes/cache/cache_test.go index 7ef5088d9b..f9bc77d890 100644 --- a/kubernetes/cache/cache_test.go +++ b/kubernetes/cache/cache_test.go @@ -1,9 +1,7 @@ package cache import ( - "context" "testing" - "time" "github.com/stretchr/testify/require" apps_v1 "k8s.io/api/apps/v1" @@ -30,42 +28,6 @@ func (f *fakeKubeCache) getClient() kubernetes.ClientInterface { return f.kubeCache.client } -func TestClientUpdatedWhenSAClientChanges(t *testing.T) { - require := require.New(t) - conf := config.NewConfig() - config.Set(conf) - - client := kubetest.NewFakeK8sClient() - client.Token = "current-token" - clientFactory := kubetest.NewK8SClientFactoryMock(client) - k8sCache, err := NewKubeCache(client, *conf) - require.NoError(err) - - kubeCache := &fakeKubeCache{kubeCache: k8sCache} - kialiCache := &kialiCacheImpl{ - clientRefreshPollingPeriod: time.Millisecond, - clientFactory: clientFactory, - KubeCache: kubeCache, - } - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - kialiCache.watchForClientChanges(ctx, client.Token) - - // Update the client. This should trigger a cache refresh. - newClient := kubetest.NewFakeK8sClient() - newClient.Token = "new-token" - clientFactory.SetClients(map[string]kubernetes.ClientInterface{conf.KubernetesConfig.ClusterName: newClient}) - - require.Eventually( - func() bool { return kubeCache.getClient() != client }, - 500*time.Millisecond, - 5*time.Millisecond, - "client and cache should have been updated", - ) -} - func TestNoHomeClusterReturnsError(t *testing.T) { require := require.New(t) conf := config.NewConfig() diff --git a/kubernetes/cache/kube_cache.go b/kubernetes/cache/kube_cache.go index 1319a6c4b7..b6db3fbf7b 100644 --- a/kubernetes/cache/kube_cache.go +++ b/kubernetes/cache/kube_cache.go @@ -62,11 +62,6 @@ type KubeCache interface { // using the Kiali Service Account client. Client() kubernetes.ClientInterface - // UpdateClient will update the client used by the cache. - // Useful for when the token is refreshed for the client. - // This causes a full refresh of the cache. - UpdateClient(client kubernetes.ClientInterface) error - GetConfigMap(namespace, name string) (*core_v1.ConfigMap, error) GetDaemonSets(namespace string) ([]apps_v1.DaemonSet, error) GetDaemonSet(namespace, name string) (*apps_v1.DaemonSet, error) diff --git a/kubernetes/client.go b/kubernetes/client.go index 57b91da266..0f927d3438 100644 --- a/kubernetes/client.go +++ b/kubernetes/client.go @@ -130,7 +130,12 @@ func getConfigForLocalCluster() (*rest.Config, error) { remoteSecretPath := kialiconfig.Get().Deployment.RemoteSecretPath if remoteSecret, readErr := GetRemoteSecret(remoteSecretPath); readErr == nil { log.Debugf("Using remote secret for local cluster config found at: [%s]. Kiali must be running outside the kube cluster.", remoteSecretPath) - return clientcmd.NewDefaultClientConfig(*remoteSecret, nil).ClientConfig() + cc, err := clientcmd.NewDefaultClientConfig(*remoteSecret, nil).ClientConfig() + if err != nil { + return cc, err + } + cc.BearerTokenFile = remoteSecretPath + return cc, nil } else { log.Debugf("Unable to read remote secret. It may or may not exist. Error: %v. Falling back to in cluster config", readErr) // Fallback to in cluster config diff --git a/kubernetes/client_factory.go b/kubernetes/client_factory.go index 3cb19e8b75..6012e92c6b 100644 --- a/kubernetes/client_factory.go +++ b/kubernetes/client_factory.go @@ -185,7 +185,7 @@ func (cf *clientFactory) newClient(authInfo *api.AuthInfo, expirationTime time.D var err error if cluster == cf.homeCluster { - kialiToken, err = GetKialiTokenForHomeCluster() + kialiToken, _, err = GetKialiTokenForHomeCluster() } else { kialiToken, err = cf.GetSAClient(cluster).GetToken(), nil } @@ -417,66 +417,11 @@ func getTokenHash(authInfo *api.AuthInfo) string { // KialiSAClients returns all clients associated with the Kiali service account across clusters. func (cf *clientFactory) GetSAClient(cluster string) ClientInterface { - // while we are here, refresh the client - if err := cf.refreshClientIfTokenChanged(cluster); err != nil { - log.Errorf("Unable to refresh Kiali SA client for cluster [%s]: %v", cluster, err) - } - cf.mutex.RLock() defer cf.mutex.RUnlock() return cf.saClientEntries[cluster] } -// Check for kiali token changes and refresh the client when it does. -func (cf *clientFactory) refreshClientIfTokenChanged(cluster string) error { - var refreshTheClient bool // will be true if the client needs to be refreshed - var rci *RemoteClusterInfo - - if cluster == cf.homeCluster { - // LOCAL CLUSTER - if newTokenToCheck, err := GetKialiTokenForHomeCluster(); err != nil { - return err - } else { - cf.mutex.RLock() - client, ok := cf.saClientEntries[cluster] - cf.mutex.RUnlock() - if !ok { - return errors.New("there is no home cluster SA client to refresh") - } - refreshTheClient = client.GetToken() != newTokenToCheck - rci = nil - } - } else { - // REMOTE CLUSTER - cf.mutex.RLock() - remoteRci, ok := cf.remoteClusterInfos[cluster] - cf.mutex.RUnlock() - if !ok { - return fmt.Errorf("cannot refresh token for unknown cluster [%s]", cluster) - } else { - if reloadedRci, err := reloadRemoteClusterInfoFromFile(remoteRci); err != nil { - return err - } else { - refreshTheClient = (reloadedRci != nil) // note that anything (not just the token) that changed will trigger the client to be refreshed - rci = reloadedRci - } - } - } - - if refreshTheClient { - log.Debugf("Kiali SA token has changed for cluster [%s], refreshing the client", cluster) - newClient, err := cf.newSAClient(rci) - if err != nil { - return err - } - cf.mutex.Lock() - cf.saClientEntries[cluster] = newClient - cf.mutex.Unlock() - } - - return nil -} - // KialiSAHomeClusterClient returns the Kiali service account client for the cluster where Kiali is running. func (cf *clientFactory) GetSAHomeClusterClient() ClientInterface { return cf.GetSAClient(cf.homeCluster) @@ -498,13 +443,14 @@ func (cf *clientFactory) getConfig(clusterInfo *RemoteClusterInfo) (*rest.Config } else { // Just read the token and then use the base config. // We're an in cluster client. Read the kiali service account token. - kialiToken, err := GetKialiTokenForHomeCluster() + kialiToken, kialiTokenFile, err := GetKialiTokenForHomeCluster() if err != nil { return nil, fmt.Errorf("unable to get Kiali service account token: %s", err) } // Copy over the base rest config and the token clientConfig.BearerToken = kialiToken + clientConfig.BearerTokenFile = kialiTokenFile } if !kialiConfig.KialiFeatureFlags.Clustering.EnableExecProvider { diff --git a/kubernetes/client_factory_test.go b/kubernetes/client_factory_test.go index 8f3ec4f21a..4f0f313766 100644 --- a/kubernetes/client_factory_test.go +++ b/kubernetes/client_factory_test.go @@ -130,63 +130,6 @@ func TestConcurrentClientFactory(t *testing.T) { wg.Wait() } -func TestSAHomeClientUpdatesWhenKialiTokenChanges(t *testing.T) { - assert := assert.New(t) - require := require.New(t) - kialiConfig := config.NewConfig() - config.Set(kialiConfig) - currentToken := KialiTokenForHomeCluster - currentTime := tokenRead - t.Cleanup(func() { - // Other tests use this global var so we need to reset it. - tokenRead = currentTime - KialiTokenForHomeCluster = currentToken - }) - - tokenRead = time.Now() - KialiTokenForHomeCluster = "current-token" - - restConfig := rest.Config{} - clientFactory, err := newClientFactory(&restConfig) - require.NoError(err) - - currentClient := clientFactory.GetSAHomeClusterClient() - assert.Equal(KialiTokenForHomeCluster, currentClient.GetToken()) - assert.Equal(currentClient, clientFactory.GetSAHomeClusterClient()) - - KialiTokenForHomeCluster = "new-token" - - // Assert that the token has changed and the client has changed. - newClient := clientFactory.GetSAHomeClusterClient() - assert.Equal(KialiTokenForHomeCluster, newClient.GetToken()) - assert.NotEqual(currentClient, newClient) -} - -func TestSAClientsUpdateWhenKialiTokenChanges(t *testing.T) { - require := require.New(t) - conf := config.NewConfig() - config.Set(conf) - t.Cleanup(func() { - // Other tests use this global var so we need to reset it. - KialiTokenForHomeCluster = "" - }) - - tokenRead = time.Now() - KialiTokenForHomeCluster = "current-token" - - restConfig := rest.Config{} - clientFactory, err := newClientFactory(&restConfig) - require.NoError(err) - - client := clientFactory.GetSAClient(conf.KubernetesConfig.ClusterName) - require.Equal(KialiTokenForHomeCluster, client.GetToken()) - - KialiTokenForHomeCluster = "new-token" - - client = clientFactory.GetSAClient(conf.KubernetesConfig.ClusterName) - require.Equal(KialiTokenForHomeCluster, client.GetToken()) -} - func TestClientCreatedWithClusterInfo(t *testing.T) { // Create a fake cluster info file. // Ensure client gets created with this. diff --git a/kubernetes/token.go b/kubernetes/token.go index 611ae70b8f..f7fde69b44 100644 --- a/kubernetes/token.go +++ b/kubernetes/token.go @@ -14,31 +14,34 @@ import ( var DefaultServiceAccountPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" var ( - KialiTokenForHomeCluster string - tokenRead time.Time + KialiTokenForHomeCluster string + KialiTokenFileForHomeCluster string + tokenRead time.Time ) -// GetKialiTokenForHomeCluster returns the Kiali SA token to be used to communicate with the local data plane k8s api endpoint. -func GetKialiTokenForHomeCluster() (string, error) { +// GetKialiTokenForHomeCluster returns the Kiali SA token to be used to communicate with the local data plane k8s api endpoint and the token file. +func GetKialiTokenForHomeCluster() (string, string, error) { // TODO: refresh the token when it changes rather than after it expires if KialiTokenForHomeCluster == "" || shouldRefreshToken() { if remoteSecret, err := GetRemoteSecret(config.Get().Deployment.RemoteSecretPath); err == nil { // for experimental feature - for when data plane is in a remote cluster currentContextAuthInfo := remoteSecret.Contexts[remoteSecret.CurrentContext].AuthInfo if authInfo, ok := remoteSecret.AuthInfos[currentContextAuthInfo]; ok { KialiTokenForHomeCluster = authInfo.Token + KialiTokenFileForHomeCluster = config.Get().Deployment.RemoteSecretPath } else { - return "", fmt.Errorf("auth info not found for current context: [%s]. Current context must be set for kiali remote secret", remoteSecret.CurrentContext) + return "", "", fmt.Errorf("auth info not found for current context: [%s]. Current context must be set for kiali remote secret", remoteSecret.CurrentContext) } } else { token, err := os.ReadFile(DefaultServiceAccountPath) if err != nil { - return "", err + return "", "", err } KialiTokenForHomeCluster = string(token) + KialiTokenFileForHomeCluster = DefaultServiceAccountPath } tokenRead = time.Now() } - return KialiTokenForHomeCluster, nil + return KialiTokenForHomeCluster, KialiTokenFileForHomeCluster, nil } // shouldRefreshToken checks to see if the local Kiali token expired. diff --git a/kubernetes/token_test.go b/kubernetes/token_test.go index 9e1b10b976..0ab8048a14 100644 --- a/kubernetes/token_test.go +++ b/kubernetes/token_test.go @@ -27,7 +27,7 @@ func TestIsTokenExpired(t *testing.T) { DefaultServiceAccountPath = tmpFileTokenExpired setupFile(t, "thisisarandomtoken", tmpFileTokenExpired) - token, err := GetKialiTokenForHomeCluster() + token, _, err := GetKialiTokenForHomeCluster() require.NoError(err) assert.True(token != "") @@ -47,7 +47,7 @@ func TestGetKialiToken(t *testing.T) { setupFile(t, data, tmpFileGetToken) - token, err := GetKialiTokenForHomeCluster() + token, _, err := GetKialiTokenForHomeCluster() require.NoError(err) assert.Equal(data, token) @@ -61,7 +61,7 @@ func TestGetKialiTokenRemoteCluster(t *testing.T) { SetConfig(t, *config) tokenRead = time.Time{} - token, err := GetKialiTokenForHomeCluster() + token, _, err := GetKialiTokenForHomeCluster() require.NoError(err) require.Equal("token2", token) diff --git a/prometheus/client.go b/prometheus/client.go index 34647cca50..2f28e93aac 100644 --- a/prometheus/client.go +++ b/prometheus/client.go @@ -82,7 +82,7 @@ func NewClientForConfig(cfg config.PrometheusConfig) (*Client, error) { // Note: if we are using the 'bearer' authentication method then we want to use the Kiali // service account token and not the user's token. This is because Kiali does filtering based // on the user's token and prevents people who shouldn't have access to particular metrics. - token, err := kubernetes.GetKialiTokenForHomeCluster() + token, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { log.Errorf("Could not read the Kiali Service Account token: %v", err) return nil, err diff --git a/status/discover.go b/status/discover.go index 252cd6b51d..9e2839d227 100644 --- a/status/discover.go +++ b/status/discover.go @@ -15,7 +15,8 @@ import ( var clientFactory kubernetes.ClientFactory func getClient() (kubernetes.ClientInterface, error) { - saToken, err := kubernetes.GetKialiTokenForHomeCluster() + // TODO: Why is this here? Shouldn't GetClientFactory() be enough? Why are we explicitly giving it the SA token? + saToken, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { return nil, err } diff --git a/status/versions.go b/status/versions.go index 812bf6ff04..7aa65672b8 100644 --- a/status/versions.go +++ b/status/versions.go @@ -222,7 +222,7 @@ func prometheusVersion() (*ExternalServiceInfo, error) { // Be sure to copy config.Auth and not modify the existing auth := cfg.Auth if auth.UseKialiToken { - token, err := kubernetes.GetKialiTokenForHomeCluster() + token, _, err := kubernetes.GetKialiTokenForHomeCluster() if err != nil { log.Errorf("Could not read the Kiali Service Account token: %v", err) return nil, err