Revert common cfgmap func and protect against failure

This reverts the callers of IstioConfigMapName back to using `cfg.external_services.istio.config_map_name` directly. It now logs failures in GetMesh to auto-detect the configmap instead of returning an error. Nothing is currently using that configuration so an error shouldn't be returned there. Related to: kiali#6669.

business/istio_certs.go

@@ -103,7 +103,7 @@ func (ics *IstioCertsService) getCertificateFromSecret(secretName, certName stri
 func (ics *IstioCertsService) getCertsConfigFromIstioConfigMap() ([]certConfig, error) {
 	cfg := config.Get()
 
-	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
+	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
 	if err != nil {
 		return nil, err
 	}
@@ -163,7 +163,7 @@ func (ics *IstioCertsService) getChironCertificates(certsConfig []certConfig) ([
 func (ics *IstioCertsService) GetTlsMinVersion() (string, error) {
 	cfg := config.Get()
 
-	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
+	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
 	if err != nil {
 		return "N/A", err
 	}

business/istio_validations.go

@@ -563,9 +563,9 @@ func (in *IstioValidationsService) fetchNonLocalmTLSConfigs(mtlsDetails *kuberne
 	var istioConfig *core_v1.ConfigMap
 	var err error
 	if IsNamespaceCached(cfg.IstioNamespace) {
-		istioConfig, err = kialiCache.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
+		istioConfig, err = kialiCache.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
 	} else {
-		istioConfig, err = userClient.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
+		istioConfig, err = userClient.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
 	}
 	if err != nil {
 		errChan <- err

business/mesh.go

@@ -114,6 +114,8 @@ func (in *MeshService) IsRemoteCluster(cluster string) (bool, error) {
 
 // GetMesh gathers information about the mesh and controlplanes running in the mesh
 // from various sources e.g. istio configmap, istiod deployment envvars, etc.
+// Don't rely on the controlplane configuration being set. This should still
+// come directly from the istio configmap for the time being.
 func (in *MeshService) GetMesh(ctx context.Context) (*Mesh, error) {
 	var end observability.EndFunc
 	ctx, end = observability.StartSpan(ctx, "GetMesh",
@@ -155,20 +157,24 @@ func (in *MeshService) GetMesh(ctx context.Context) (*Mesh, error) {
 					Revision: istiod.Labels[IstioRevisionLabel],
 				}
 
-				configMapName := IstioConfigMapName(in.conf, controlPlane.Revision)
-
+				// Note that nothing is currently using this controlplane configuration so no error is being returned.
+				// TODO: When this configuration is used, auto-detection of the configmap should probably go behind
+				// a feature flag or another config option to avoid breaking existing users.
+				configMapName := guessIstioConfigMapName(controlPlane.Revision)
 				controlPlaneConfig, err := getControlPlaneConfiguration(kubeCache, istiod.Namespace, configMapName)
 				if err != nil {
-					return nil, err
-				}
-				controlPlane.Config = *controlPlaneConfig
-
-				// Kiali only supports a single mesh. All controlplanes should share the same mesh id.
-				// Otherwise this is an error.
-				if mesh.ID == nil {
-					mesh.ID = &controlPlane.Config.DefaultConfig.MeshId
-				} else if *mesh.ID != controlPlane.Config.DefaultConfig.MeshId {
-					return nil, fmt.Errorf("multiple mesh ids found: [%s] and [%s]", *mesh.ID, controlPlane.Config.DefaultConfig.MeshId)
+					log.Debugf("Unable to get controlplane config from the istio configmap for controlplane [%s/%s] on cluster [%s]: %s",
+						istiod.Name, istiod.Namespace, cluster.Name, err)
+				} else {
+					controlPlane.Config = *controlPlaneConfig
+
+					// Kiali only supports a single mesh. All controlplanes should share the same mesh id.
+					// Otherwise this is an error.
+					if mesh.ID == nil {
+						mesh.ID = &controlPlane.Config.DefaultConfig.MeshId
+					} else if *mesh.ID != controlPlane.Config.DefaultConfig.MeshId {
+						return nil, fmt.Errorf("multiple mesh ids found: [%s] and [%s]", *mesh.ID, controlPlane.Config.DefaultConfig.MeshId)
+					}
 				}
 
 				if containers := istiod.Spec.Template.Spec.Containers; len(containers) > 0 {
@@ -223,16 +229,8 @@ func (in *MeshService) GetMesh(ctx context.Context) (*Mesh, error) {
 	return mesh, nil
 }
 
-// IstioConfigMapName guesses the istio configmap name.
-func IstioConfigMapName(conf config.Config, revision string) string {
-	// If the config map name is explicitly set and it's not the default value, we should always use that.
-	// Note that this means that the revision is ignored and every controlplane
-	// will use this configmap regardless of which configmap actually corresponds
-	// to the revision.
-	if conf.ExternalServices.Istio.ConfigMapName != "" && conf.ExternalServices.Istio.ConfigMapName != "istio" {
-		return conf.ExternalServices.Istio.ConfigMapName
-	}
-
+// guessIstioConfigMapName guesses the istio configmap name.
+func guessIstioConfigMapName(revision string) string {
 	// If the revision is set, we should use the revisioned configmap name
 	// otherwise the hardcoded 'istio' value is used.
 	configMapName := "istio" // As of 1.19 this is hardcoded in the helm charts.
@@ -485,7 +483,7 @@ func (in *MeshService) resolveNetwork(clusterName string) string {
 
 func (in *MeshService) OutboundTrafficPolicy() (*models.OutboundPolicy, error) {
 	otp := models.OutboundPolicy{Mode: "ALLOW_ANY"}
-	istioConfig, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, IstioConfigMapName(in.conf, ""))
+	istioConfig, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, in.conf.ExternalServices.Istio.ConfigMapName)
 	if err != nil {
 		return nil, err
 	}

business/mesh_internal_test.go

@@ -0,0 +1,34 @@
+package business
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIstioConfigMapName(t *testing.T) {
+	testCases := map[string]struct {
+		revision string
+		expected string
+	}{
+		"revision is default": {
+			revision: "default",
+			expected: "istio",
+		},
+		"revision is v1": {
+			revision: "v1",
+			expected: "istio-v1",
+		},
+		"revision is empty": {
+			revision: "",
+			expected: "istio",
+		},
+	}
+
+	for desc, tc := range testCases {
+		t.Run(desc, func(t *testing.T) {
+			result := guessIstioConfigMapName(tc.revision)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}

business/mesh_test.go

@@ -628,6 +628,34 @@ trustDomain: cluster.local
 	require.Len(mesh.ControlPlanes[0].ManagedClusters, 1)
 }
 
+// Ensure that fetching the configmap configuration won't produce failures due to regression: https://github.com/kiali/kiali/issues/6669
+func TestGetMeshSucceedsEvenWhenFetchingIstioConfigFails(t *testing.T) {
+	require := require.New(t)
+	conf := config.NewConfig()
+	kubernetes.SetConfig(t, *conf)
+
+	istiodDeployment := &apps_v1.Deployment{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "istiod",
+			Namespace: "istio-system",
+			Labels:    map[string]string{"app": "istiod"},
+		},
+	}
+	k8s := kubetest.NewFakeK8sClient(
+		&core_v1.Namespace{ObjectMeta: v1.ObjectMeta{Name: "istio-system"}},
+		istiodDeployment,
+	)
+
+	business.SetupBusinessLayer(t, k8s, *conf)
+
+	clients := map[string]kubernetes.ClientInterface{conf.KubernetesConfig.ClusterName: k8s}
+	svc := business.NewWithBackends(clients, clients, nil, nil).Mesh
+	mesh, err := svc.GetMesh(context.TODO())
+	require.NoError(err)
+	require.Len(mesh.ControlPlanes, 1)
+	require.Len(mesh.ControlPlanes[0].ManagedClusters, 1)
+}
+
 func TestGetMeshMultipleRevisions(t *testing.T) {
 	istiod_1_18_Deployment := &apps_v1.Deployment{
 		ObjectMeta: v1.ObjectMeta{
@@ -703,18 +731,6 @@ trustDomain: cluster.local
 	})
 	require.True(*controlPlane_1_19.Config.EnableAutoMtls)
 	require.Len(controlPlane_1_19.ManagedClusters, 1)
-
-	// Test for setting the configmap name explicitly due to regression: https://github.com/kiali/kiali/issues/6669
-	conf.ExternalServices.Istio.ConfigMapName = istio_1_19_ConfigMap.Name
-	config.Set(conf)
-	svc = business.NewWithBackends(clients, clients, nil, nil).Mesh
-	mesh, err = svc.GetMesh(context.TODO())
-	require.NoError(err)
-
-	require.Len(mesh.ControlPlanes, 2)
-	// Both controlplanes should set this to true since both will use the 1.19 configmap.
-	require.True(*mesh.ControlPlanes[0].Config.EnableAutoMtls)
-	require.True(*mesh.ControlPlanes[1].Config.EnableAutoMtls)
 }
 
 func TestGetMeshRemoteClusters(t *testing.T) {
@@ -1044,74 +1060,3 @@ func fakeIstiodDeployment(cluster string, manageExternal bool) *apps_v1.Deployme
 	}
 	return deployment
 }
-
-func TestIstioConfigMapName(t *testing.T) {
-	testCases := map[string]struct {
-		conf     config.Config
-		revision string
-		expected string
-	}{
-		"ConfigMapName is empty and revision is default": {
-			conf: config.Config{
-				ExternalServices: config.ExternalServices{
-					Istio: config.IstioConfig{
-						ConfigMapName: "",
-					},
-				},
-			},
-			revision: "default",
-			expected: "istio",
-		},
-		"ConfigMapName is empty and revision is v1": {
-			conf: config.Config{
-				ExternalServices: config.ExternalServices{
-					Istio: config.IstioConfig{
-						ConfigMapName: "",
-					},
-				},
-			},
-			revision: "v1",
-			expected: "istio-v1",
-		},
-		"ConfigMapName is set and revision is default": {
-			conf: config.Config{
-				ExternalServices: config.ExternalServices{
-					Istio: config.IstioConfig{
-						ConfigMapName: "my-istio-config",
-					},
-				},
-			},
-			revision: "default",
-			expected: "my-istio-config",
-		},
-		"ConfigMapName is set and revision is v2": {
-			conf: config.Config{
-				ExternalServices: config.ExternalServices{
-					Istio: config.IstioConfig{
-						ConfigMapName: "my-istio-config",
-					},
-				},
-			},
-			revision: "v2",
-			expected: "my-istio-config",
-		},
-		"ConfigMapName is set and revision is empty": {
-			conf: config.Config{
-				ExternalServices: config.ExternalServices{
-					Istio: config.IstioConfig{
-						ConfigMapName: "my-istio-config",
-					},
-				},
-			},
-			revision: "",
-			expected: "my-istio-config",
-		},
-	}
-
-	for desc, tc := range testCases {
-		t.Run(desc, func(t *testing.T) {
-			result := business.IstioConfigMapName(tc.conf, tc.revision)
-			assert.Equal(t, tc.expected, result)
-		})
-	}
-}

business/namespaces.go

@@ -95,7 +95,7 @@ func (in *NamespaceService) GetNamespaces(ctx context.Context) ([]models.Namespa
 
 	// determine what the discoverySelectors are by examining the Istio ConfigMap
 	var discoverySelectors []*meta_v1.LabelSelector
-	if icm, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, IstioConfigMapName(in.conf, "")); err == nil {
+	if icm, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, in.conf.ExternalServices.Istio.ConfigMapName); err == nil {
 		if ic, err2 := kubernetes.GetIstioConfigMap(icm); err2 == nil {
 			discoverySelectors = ic.DiscoverySelectors
 		} else {

business/tls.go

@@ -142,7 +142,7 @@ func (in *TLSService) hasAutoMTLSEnabled(cluster string) bool {
 	}
 
 	cfg := config.Get()
-	istioConfig, err := kubeCache.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
+	istioConfig, err := kubeCache.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
 	if err != nil {
 		return true
 	}