Repository: kiali/kiali
Pull request #7580 (2024-08-01 09:23:21)

Move to go 1.22.5
upgrade go from 1.22.4 to 1.22.5


Patch:
diff --git a/config/config.go b/config/config.go
index 8d80c8046..002d89b55 100644
--- a/config/config.go
+++ b/config/config.go
@@ -968,6 +968,11 @@ func (conf *Config) GatewayLabel(labelConfig string) []string {
 	return []string{}
 }
 
+func (conf *Config) IsRBACDisabled() bool {
+	return conf.Auth.Strategy == AuthStrategyAnonymous ||
+		(conf.Auth.Strategy == AuthStrategyOpenId && conf.Auth.OpenId.DisableRBAC)
+}
+
 // Get the global Config
 func Get() (conf *Config) {
 	rwMutex.RLock()
diff --git a/go.mod b/go.mod
index 3bb83c63a..cd62c307a 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,6 @@
 module github.com/kiali/kiali
 
-go 1.22.4
+go 1.22.5
 
 require (
 	github.com/NYTimes/gziphandler v1.1.1
diff --git a/kubernetes/cache/cache.go b/kubernetes/cache/cache.go
index fa319c2c1..fa4a2664d 100644
--- a/kubernetes/cache/cache.go
+++ b/kubernetes/cache/cache.go
@@ -26,8 +26,10 @@ const (
 	waypointExpirationTime     = 1 * time.Minute
 )
 
-const kialiCacheMeshKey = "mesh"
-const ztunnelApp = "ztunnel"
+const (
+	kialiCacheMeshKey = "mesh"
+	ztunnelApp        = "ztunnel"
+)
 
 // KialiCache stores both kube objects and non-kube related data such as pods' proxy status.
 // It is exclusively used by the business layer where it's expected to be a singleton.
@@ -307,6 +309,10 @@ type namespacesKey struct {
 	token   string
 }
 
+func (n namespacesKey) String() string {
+	return fmt.Sprintf("cluster: %s\ttoken: xxx", n.cluster)
+}
+
 func (c *kialiCacheImpl) GetNamespace(cluster string, token string, namespace string) (models.Namespace, bool) {
 	c.namespacesLock.RLock()
 	defer c.namespacesLock.RUnlock()
diff --git a/kubernetes/client_factory.go b/kubernetes/client_factory.go
index 40144b9f1..2ce684300 100644
--- a/kubernetes/client_factory.go
+++ b/kubernetes/client_factory.go
@@ -49,6 +49,8 @@ type clientFactory struct {
 	// "in cluster" config. This name comes from the istio cluster id.
 	homeCluster string
 
+	kialiConfig *kialiConfig.Config
+
 	// mutex for when accessing the stored clients
 	mutex sync.RWMutex
 
@@ -92,7 +94,7 @@ func getClientFactory(conf kialiConfig.Config) (*clientFactory, error) {
 		Burst:           conf.KubernetesConfig.Burst,
 	}
 
-	return newClientFactory(&baseConfig)
+	return newClientFactory(&conf, &baseConfig)
 }
 
 // NewClientFactory creates a new client factory that can be transitory.
@@ -118,8 +120,9 @@ func NewClientFactory(ctx context.Context, conf kialiConfig.Config) (ClientFacto
 
 // newClientFactory allows for specifying the config and expiry duration
 // Mock friendly for testing purposes
-func newClientFactory(restConfig *rest.Config) (*clientFactory, error) {
+func newClientFactory(conf *kialiConfig.Config, restConfig *rest.Config) (*clientFactory, error) {
 	f := &clientFactory{
+		kialiConfig:     conf,
 		baseRestConfig:  restConfig,
 		clientEntries:   make(map[string]map[string]ClientInterface),
 		recycleChan:     make(chan string),
@@ -179,40 +182,22 @@ func (cf *clientFactory) newClient(authInfo *api.AuthInfo, expirationTime time.D
 	// So, if OpenID strategy is active, check if a proxy is configured.
 	// If there is, use it UNLESS the token is the one of the Kiali SA. If
 	// the token is the one of the Kiali SA, the proxy can be bypassed.
-	cfg := kialiConfig.Get()
-	if cfg.Auth.Strategy == kialiConfig.AuthStrategyOpenId && cfg.Auth.OpenId.ApiProxy != "" && cfg.Auth.OpenId.ApiProxyCAData != "" {
-
-		var kialiToken string
-		var err error
-
-		if cluster == cf.homeCluster {
-			kialiToken, _, err = GetKialiTokenForHomeCluster()
-		} else {
-			kialiToken, err = cf.GetSAClient(cluster).GetToken(), nil
-		}
-
+	if cf.kialiConfig.Auth.Strategy == kialiConfig.AuthStrategyOpenId && cf.kialiConfig.Auth.OpenId.ApiProxy != "" && cf.kialiConfig.Auth.OpenId.ApiProxyCAData != "" {
+		// Override the CA data on the client with the proxy CA from the Kiali config.
+		caData := cf.kialiConfig.Auth.OpenId.ApiProxyCAData
+		rootCaDecoded, err := base64.StdEncoding.DecodeString(caData)
 		if err != nil {
 			return nil, err
 		}
 
-		// User token and not Kiali SA token so use the proxy.
-		if kialiToken != authInfo.Token {
-			// Override the CA data on the client with the proxy CA from the Kiali config.
-			caData := cfg.Auth.OpenId.ApiProxyCAData
-			rootCaDecoded, err := base64.StdEncoding.DecodeString(caData)
-			if err != nil {
-				return nil, err
-			}
-
-			config.TLSClientConfig = rest.TLSClientConfig{
-				CAData: []byte(rootCaDecoded),
-			}
-			config.Host = cfg.Auth.OpenId.ApiProxy
+		config.TLSClientConfig = rest.TLSClientConfig{
+			CAData: []byte(rootCaDecoded),
 		}
+		config.Host = cf.kialiConfig.Auth.OpenId.ApiProxy
 	}
 
 	// Impersonation is valid only for header authentication strategy
-	if cfg.Auth.Strategy == kialiConfig.AuthStrategyHeader && authInfo.Impersonate != "" {
+	if cf.kialiConfig.Auth.Strategy == kialiConfig.AuthStrategyHeader && authInfo.Impersonate != "" {
 		config.Impersonate.UserName = authInfo.Impersonate
 		config.Impersonate.Groups = authInfo.ImpersonateGroups
 		config.Impersonate.Extra = authInfo.ImpersonateUserExtra
@@ -246,13 +231,8 @@ func (cf *clientFactory) newClient(authInfo *api.AuthInfo, expirationTime time.D
 		}
 
 		// Replace the Kiali SA token with the user's auth token.
-		// Only if we are not in an anonymous mode
-		// and if we don't use OpenID with RBAC is disable.
-		if !(cfg.Auth.Strategy == kialiConfig.AuthStrategyAnonymous) &&
-			!(cfg.Auth.Strategy == kialiConfig.AuthStrategyOpenId && cfg.Auth.OpenId.DisableRBAC) {
-			remoteConfig.BearerToken = authInfo.Token
-			remoteConfig.BearerTokenFile = ""
-		}
+		remoteConfig.BearerToken = authInfo.Token
+		remoteConfig.BearerTokenFile = ""
 
 		newClient, err = newClientWithRemoteClusterInfo(remoteConfig, &clusterInfo)
 		if err != nil {
@@ -296,11 +276,19 @@ func (cf *clientFactory) GetSAClients() map[string]ClientInterface {
 
 // getClient returns a client for the specified token. Creating one if necessary.
 func (cf *clientFactory) GetClient(authInfo *api.AuthInfo, cluster string) (ClientInterface, error) {
+	if cf.kialiConfig.IsRBACDisabled() {
+		return cf.GetSAClient(cluster), nil
+	}
+
 	return cf.getRecycleClient(authInfo, defaultExpirationTime, cluster)
 }
 
 // getClient returns a client for the specified token. Creating one if necessary.
 func (cf *clientFactory) GetClients(authInfos map[string]*api.AuthInfo) (map[string]ClientInterface, error) {
+	if cf.kialiConfig.IsRBACDisabled() {
+		return cf.GetSAClients(), nil
+	}
+
 	clients := make(map[string]ClientInterface)
 	for cluster, authInfo := range authInfos {
 		ci, err := cf.getRecycleClient(authInfo, defaultExpirationTime, cluster)
diff --git a/store/expiration_store.go b/store/expiration_store.go
index fdd80ba96..0f00e98d4 100644
--- a/store/expiration_store.go
+++ b/store/expiration_store.go
@@ -90,7 +90,7 @@ func (s *ExpirationStore[K, V]) checkAndRemoveExpiredKeys(ctx context.Context) <
 					}
 
 					if time.Now().After(expiration) {
-						log.Tracef("Key %v expired. Removing from store", key)
+						log.Tracef("Key '%v' expired. Removing from store", key)
 						s.Remove(key)
 					}
 				}


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 16e0dc23d..85fd4cfd7 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/kiali/kiali/util"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSecretFileOverrides(t *testing.T) {
@@ -366,3 +367,63 @@ func TestValidateAuthStrategy(t *testing.T) {
 		}
 	}
 }
+
+func TestIsRBACDisabled(t *testing.T) {
+	cases := map[string]struct {
+		authConfig         AuthConfig
+		expectRBACDisabled bool
+	}{
+		"anonymous should have RBAC disabled": {
+			authConfig: AuthConfig{
+				Strategy: AuthStrategyAnonymous,
+			},
+			expectRBACDisabled: true,
+		},
+		"openid with rbac disabled should have RBAC disabled": {
+			authConfig: AuthConfig{
+				Strategy: AuthStrategyOpenId,
+				OpenId: OpenIdConfig{
+					DisableRBAC: true,
+				},
+			},
+			expectRBACDisabled: true,
+		},
+		"openid with rbac enabled should have RBAC enabled": {
+			authConfig: AuthConfig{
+				Strategy: AuthStrategyOpenId,
+				OpenId: OpenIdConfig{
+					DisableRBAC: false,
+				},
+			},
+			expectRBACDisabled: false,
+		},
+		"openshift should have RBAC enabled": {
+			authConfig: AuthConfig{
+				Strategy: AuthStrategyOpenshift,
+			},
+			expectRBACDisabled: false,
+		},
+		"token should have RBAC enabled": {
+			authConfig: AuthConfig{
+				Strategy: AuthStrategyToken,
+			},
+			expectRBACDisabled: false,
+		},
+		"header should have RBAC enabled": {
+			authConfig: AuthConfig{
+				Strategy: AuthStrategyHeader,
+			},
+			expectRBACDisabled: false,
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			require := require.New(t)
+
+			conf := NewConfig()
+			conf.Auth = tc.authConfig
+
+			require.Equal(tc.expectRBACDisabled, conf.IsRBACDisabled())
+		})
+	}
+}
diff --git a/kubernetes/client_factory_test.go b/kubernetes/client_factory_test.go
index 3692bf18e..b6ba74974 100644
--- a/kubernetes/client_factory_test.go
+++ b/kubernetes/client_factory_test.go
@@ -114,6 +114,7 @@ func TestConcurrentClientExpiration(t *testing.T) {
 // TestConcurrentClientFactory test Concurrently create ClientFactory
 func TestConcurrentClientFactory(t *testing.T) {
 	require := require.New(t)
+	conf := config.NewConfig()
 	istioConfig := rest.Config{}
 	count := 100
 
@@ -125,7 +126,7 @@ func TestConcurrentClientFactory(t *testing.T) {
 	for i := 0; i < count; i++ {
 		go func() {
 			defer wg.Done()
-			_, err := newClientFactory(&istioConfig)
+			_, err := newClientFactory(conf, &istioConfig)
 			require.NoError(err)
 		}()
 	}
@@ -387,39 +388,73 @@ func setGlobalKialiSAToken(t *testing.T) {
 }
 
 func TestClientCreatedWithProxyInfo(t *testing.T) {
-	require := require.New(t)
-
-	cfg := config.NewConfig()
-	cfg.Deployment.RemoteSecretPath = t.TempDir() // Random dir so that the remote secret isn't read if it exists.
-	cfg.Auth.Strategy = config.AuthStrategyOpenId
-	cfg.Auth.OpenId.ApiProxyCAData = base64.StdEncoding.EncodeToString(proxyCAData)
-	cfg.Auth.OpenId.ApiProxy = "https://api-proxy:8443"
-	SetConfig(t, *cfg)
-
-	clientFactory := NewTestingClientFactory(t)
+	cases := map[string]struct {
+		auth              config.AuthConfig
+		expectProxyConfig bool
+	}{
+		"openid with proxy defined should have proxy on client": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyOpenId,
+				OpenId: config.OpenIdConfig{
+					ApiProxy:       "https://api-proxy:8443",
+					ApiProxyCAData: base64.StdEncoding.EncodeToString(proxyCAData),
+				},
+			},
+			expectProxyConfig: true,
+		},
+		"openid with rbac disabled and proxy defined should not have proxy on client": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyOpenId,
+				OpenId: config.OpenIdConfig{
+					DisableRBAC:    true,
+					ApiProxy:       "https://api-proxy:8443",
+					ApiProxyCAData: base64.StdEncoding.EncodeToString(proxyCAData),
+				},
+			},
+			expectProxyConfig: false,
+		},
+		"anonymous with proxy defined should not have proxy on client": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyAnonymous,
+				OpenId: config.OpenIdConfig{
+					DisableRBAC:    true,
+					ApiProxy:       "https://api-proxy:8443",
+					ApiProxyCAData: base64.StdEncoding.EncodeToString(proxyCAData),
+				},
+			},
+			expectProxyConfig: false,
+		},
+	}
 
-	// Regular clients should have the proxy info
-	client, err := clientFactory.GetClient(api.NewAuthInfo(), cfg.KubernetesConfig.ClusterName)
-	require.NoError(err)
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			require := require.New(t)
 
-	require.Equal(cfg.Auth.OpenId.ApiProxy, client.ClusterInfo().ClientConfig.Host)
-	require.Equal(proxyCAData, client.ClusterInfo().ClientConfig.CAData)
+			cfg := config.NewConfig()
+			cfg.Deployment.RemoteSecretPath = t.TempDir() // Random dir so that the remote secret isn't read if it exists.
+			cfg.Auth = tc.auth
+			SetConfig(t, *cfg)
 
-	// Service account clients should not have the proxy info.
-	// Two ways to get a SA client: 1. GetClient with SA token and 2. GetSAClient
-	setGlobalKialiSAToken(t)
-	authInfo := api.NewAuthInfo()
-	authInfo.Token = KialiTokenForHomeCluster
+			clientFactory := NewTestingClientFactory(t)
 
-	client, err = clientFactory.GetClient(authInfo, cfg.KubernetesConfig.ClusterName)
-	require.NoError(err)
+			// Regular clients should have the proxy info
+			client, err := clientFactory.GetClient(api.NewAuthInfo(), cfg.KubernetesConfig.ClusterName)
+			require.NoError(err)
 
-	require.NotEqual(cfg.Auth.OpenId.ApiProxy, client.ClusterInfo().ClientConfig.Host)
-	require.NotEqual(proxyCAData, client.ClusterInfo().ClientConfig.CAData)
+			if tc.expectProxyConfig {
+				require.Equal(cfg.Auth.OpenId.ApiProxy, client.ClusterInfo().ClientConfig.Host)
+				require.Equal(proxyCAData, client.ClusterInfo().ClientConfig.CAData)
+			} else {
+				require.NotEqual(cfg.Auth.OpenId.ApiProxy, client.ClusterInfo().ClientConfig.Host)
+				require.NotEqual(proxyCAData, client.ClusterInfo().ClientConfig.CAData)
+			}
 
-	client = clientFactory.GetSAClient(cfg.KubernetesConfig.ClusterName)
-	require.NotEqual(cfg.Auth.OpenId.ApiProxy, client.ClusterInfo().ClientConfig.Host)
-	require.NotEqual(proxyCAData, client.ClusterInfo().ClientConfig.CAData)
+			// Service account clients should not have the proxy info.
+			client = clientFactory.GetSAClient(cfg.KubernetesConfig.ClusterName)
+			require.NotEqual(cfg.Auth.OpenId.ApiProxy, client.ClusterInfo().ClientConfig.Host)
+			require.NotEqual(proxyCAData, client.ClusterInfo().ClientConfig.CAData)
+		})
+	}
 }
 
 func TestNewClientFactoryClosesRecycleWhenCTXCancelled(t *testing.T) {
@@ -506,3 +541,69 @@ func TestClientFactoryReturnsSAClientWhenConfigClusterNameIsEmpty(t *testing.T)
 
 	require.NotNil(clientFactory.GetSAHomeClusterClient())
 }
+
+func TestClientFactoryGetClients(t *testing.T) {
+	cases := map[string]struct {
+		auth               config.AuthConfig
+		authInfo           map[string]*api.AuthInfo
+		expectClientsEqual bool
+	}{
+		"anonymous strategy returns Service Account clients": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyAnonymous,
+			},
+			authInfo:           map[string]*api.AuthInfo{"cluster1": api.NewAuthInfo()},
+			expectClientsEqual: true,
+		},
+		"anonymous strategy returns Service Account clients with nil auth info": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyAnonymous,
+			},
+			authInfo:           nil,
+			expectClientsEqual: true,
+		},
+		"openid with rbac disabled returns Service Account clients": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyOpenId,
+				OpenId: config.OpenIdConfig{
+					DisableRBAC: true,
+				},
+			},
+			authInfo:           map[string]*api.AuthInfo{"cluster1": api.NewAuthInfo()},
+			expectClientsEqual: true,
+		},
+		"openid with rbac enabled returns user clients": {
+			auth: config.AuthConfig{
+				Strategy: config.AuthStrategyOpenId,
+				OpenId: config.OpenIdConfig{
+					DisableRBAC: false,
+				},
+			},
+			authInfo:           map[string]*api.AuthInfo{"cluster1": api.NewAuthInfo()},
+			expectClientsEqual: false,
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			require := require.New(t)
+			conf := config.NewConfig()
+			conf.Auth = tc.auth
+			conf.KubernetesConfig.ClusterName = "cluster1"
+			SetConfig(t, *conf)
+
+			clientFactory := NewTestingClientFactory(t)
+			clients, err := clientFactory.GetClients(tc.authInfo)
+			require.NoError(err)
+			require.Len(clients, 1)
+
+			saClients := clientFactory.GetSAClients()
+			require.Len(saClients, 1)
+
+			if tc.expectClientsEqual {
+				require.Equal(clients["cluster1"], saClients["cluster1"])
+			} else {
+				require.NotEqual(clients["cluster1"], saClients["cluster1"])
+			}
+		})
+	}
+}
diff --git a/kubernetes/testing.go b/kubernetes/testing.go
index c86456d6c..1bd58dfa6 100644
--- a/kubernetes/testing.go
+++ b/kubernetes/testing.go
@@ -56,7 +56,7 @@ func NewTestingClientFactory(t *testing.T) *clientFactory {
 	}
 
 	clientConfig := rest.Config{}
-	client, err := newClientFactory(&clientConfig)
+	client, err := newClientFactory(config.Get(), &clientConfig)
 	if err != nil {
 		t.Fatalf("Error creating client factory: %v", err)
 	}