Fix double revision in istio configmap name (#6675)

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, cfg.ExternalServices.Istio.ConfigMapName)
+	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
 	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, cfg.ExternalServices.Istio.ConfigMapName)
+	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
 	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, cfg.ExternalServices.Istio.ConfigMapName)
+		istioConfig, err = kialiCache.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
 	} else {
-		istioConfig, err = userClient.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
+		istioConfig, err = userClient.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
 	}
 	if err != nil {
 		errChan <- err

business/istio_validations_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 	networking_v1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
 	security_v1beta "istio.io/client-go/pkg/apis/security/v1beta1"
+	apps_v1 "k8s.io/api/apps/v1"
 	core_v1 "k8s.io/api/core/v1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -67,11 +68,92 @@ func TestGatewayValidation(t *testing.T) {
 	conf := config.NewConfig()
 	config.Set(conf)
 
-	v := mockMultiNamespaceGatewaysValidationService(t)
+	v := mockMultiNamespaceGatewaysValidationService(t, *conf)
 	validations, _, _ := v.GetIstioObjectValidations(context.TODO(), conf.KubernetesConfig.ClusterName, "test", "gateways", "first")
 	assert.NotEmpty(validations)
 }
 
+// TestGatewayValidationScopesToNamespaceWhenGatewayToNamespaceSet this test ensures that gateway validation
+// scopes the gateway workload checker to the namespace of the gateway when PILOT_SCOPE_GATEWAY_TO_NAMESPACE
+// is set to true on the istiod deployment.
+func TestGatewayValidationScopesToNamespaceWhenGatewayToNamespaceSet(t *testing.T) {
+	assert := assert.New(t)
+	require := require.New(t)
+	const (
+		istioConfigMapName                = "istio-1-19-0"
+		istioSidecarInjectorConfigMapName = "istio-sidecar-injector-1-19-0"
+		istiodDeploymentName              = "istiod-1-19-0"
+	)
+	conf := config.NewConfig()
+	conf.ExternalServices.Istio.ConfigMapName = istioConfigMapName
+	conf.ExternalServices.Istio.IstioSidecarInjectorConfigMapName = istioSidecarInjectorConfigMapName
+	conf.ExternalServices.Istio.IstiodDeploymentName = istiodDeploymentName
+	config.Set(conf)
+	revConfigMap := &core_v1.ConfigMap{ObjectMeta: meta_v1.ObjectMeta{Name: istioConfigMapName, Namespace: "istio-system"}}
+	injectorConfigMap := &core_v1.ConfigMap{ObjectMeta: meta_v1.ObjectMeta{Name: istioSidecarInjectorConfigMapName, Namespace: "istio-system"}}
+	istioSystemNamespace := &core_v1.Namespace{ObjectMeta: meta_v1.ObjectMeta{Name: "istio-system"}}
+
+	istiod_1_19_0 := &apps_v1.Deployment{
+		ObjectMeta: meta_v1.ObjectMeta{
+			Name:      istiodDeploymentName,
+			Namespace: "istio-system",
+			Labels: map[string]string{
+				IstioRevisionLabel: "1-19-0",
+				"app":              "istiod",
+			},
+		},
+		Spec: apps_v1.DeploymentSpec{
+			Template: core_v1.PodTemplateSpec{
+				Spec: core_v1.PodSpec{
+					Containers: []core_v1.Container{
+						{
+							Env: []core_v1.EnvVar{
+								{
+									Name:  "PILOT_SCOPE_GATEWAY_TO_NAMESPACE",
+									Value: "true",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	// The gateway workload is in a different namespace than the Gateway object.
+	gatewayDeployment := &apps_v1.Deployment{
+		ObjectMeta: meta_v1.ObjectMeta{
+			Name:      "istio-ingressgateway",
+			Namespace: "istio-system",
+			Labels: map[string]string{
+				"app": "real", // Matches the gateway label selector
+			},
+		},
+		Spec: apps_v1.DeploymentSpec{
+			Template: core_v1.PodTemplateSpec{
+				ObjectMeta: meta_v1.ObjectMeta{
+					Labels: map[string]string{
+						"app": "real", // Matches the gateway label selector
+					},
+				},
+			},
+		},
+	}
+
+	v := mockMultiNamespaceGatewaysValidationService(t, *conf, revConfigMap, injectorConfigMap, istioSystemNamespace, istiod_1_19_0, gatewayDeployment)
+	validations, _, err := v.GetIstioObjectValidations(context.TODO(), conf.KubernetesConfig.ClusterName, "test", "gateways", "first")
+	require.NoError(err)
+	require.Len(validations, 1)
+	key := models.IstioValidationKey{
+		ObjectType: "gateway",
+		Name:       "first",
+		Namespace:  "test",
+	}
+	// Even though the workload is reference properly, because of the PILOT_SCOPE_GATEWAY_TO_NAMESPACE
+	// the gateway should be marked as invalid.
+	assert.False(validations[key].Valid)
+}
+
 func TestFilterExportToNamespacesVS(t *testing.T) {
 	assert := assert.New(t)
 	conf := config.NewConfig()
@@ -142,7 +224,7 @@ func TestGetVSReferencesNotExisting(t *testing.T) {
 	assert.Nil(references)
 }
 
-func mockMultiNamespaceGatewaysValidationService(t *testing.T) IstioValidationsService {
+func mockMultiNamespaceGatewaysValidationService(t *testing.T, cfg config.Config, objects ...runtime.Object) IstioValidationsService {
 	fakeIstioObjects := []runtime.Object{
 		&core_v1.ConfigMap{ObjectMeta: meta_v1.ObjectMeta{Name: "istio", Namespace: "istio-system"}},
 	}
@@ -165,16 +247,18 @@ func mockMultiNamespaceGatewaysValidationService(t *testing.T) IstioValidationsS
 		fakeIstioObjects = append(fakeIstioObjects, p.DeepCopyObject())
 	}
 
+	fakeIstioObjects = append(fakeIstioObjects, objects...)
+
 	k8s := kubetest.NewFakeK8sClient(fakeIstioObjects...)
-	cache := SetupBusinessLayer(t, k8s, *config.NewConfig())
+	cache := SetupBusinessLayer(t, k8s, cfg)
 	cache.SetRegistryStatus(&kubernetes.RegistryStatus{
 		Configuration: &kubernetes.RegistryConfiguration{
 			Gateways: append(getGateway("first", "test"), getGateway("second", "test2")...),
 		},
 	})
 
 	k8sclients := make(map[string]kubernetes.ClientInterface)
-	k8sclients[config.Get().KubernetesConfig.ClusterName] = k8s
+	k8sclients[cfg.KubernetesConfig.ClusterName] = k8s
 	return IstioValidationsService{userClients: k8sclients, businessLayer: NewWithBackends(k8sclients, k8sclients, nil, nil)}
 }
 

business/mesh.go

@@ -155,10 +155,7 @@ func (in *MeshService) GetMesh(ctx context.Context) (*Mesh, error) {
 					Revision: istiod.Labels[IstioRevisionLabel],
 				}
 
-				configMapName := in.conf.ExternalServices.Istio.ConfigMapName
-				if revLabel := istiod.Labels[IstioRevisionLabel]; revLabel != "default" && revLabel != "" {
-					configMapName = configMapName + "-" + revLabel
-				}
+				configMapName := IstioConfigMapName(in.conf, controlPlane.Revision)
 
 				controlPlaneConfig, err := getControlPlaneConfiguration(kubeCache, istiod.Namespace, configMapName)
 				if err != nil {
@@ -226,6 +223,26 @@ 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
+	}
+
+	// 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.
+	if revision != "default" && revision != "" {
+		configMapName = configMapName + "-" + revision
+	}
+
+	return configMapName
+}
+
 func envVarIsSet(key string, env core_v1.EnvVar) bool {
 	return env.Name == key && env.Value == "true"
 }
@@ -468,7 +485,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, in.conf.ExternalServices.Istio.ConfigMapName)
+	istioConfig, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, IstioConfigMapName(in.conf, ""))
 	if err != nil {
 		return nil, err
 	}

business/mesh_test.go

@@ -675,6 +675,7 @@ trustDomain: cluster.local
 	}
 	require := require.New(t)
 	conf := config.NewConfig()
+	config.Set(conf)
 	k8s := kubetest.NewFakeK8sClient(
 		&core_v1.Namespace{ObjectMeta: v1.ObjectMeta{Name: "istio-system"}},
 		istiod_1_18_Deployment,
@@ -683,7 +684,7 @@ trustDomain: cluster.local
 		istio_1_19_ConfigMap,
 	)
 
-	business.SetupBusinessLayer(t, k8s, *config.NewConfig())
+	business.SetupBusinessLayer(t, k8s, *conf)
 
 	clients := map[string]kubernetes.ClientInterface{conf.KubernetesConfig.ClusterName: k8s}
 	svc := business.NewWithBackends(clients, clients, nil, nil).Mesh
@@ -702,6 +703,18 @@ 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) {
@@ -1031,3 +1044,74 @@ 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, in.conf.ExternalServices.Istio.ConfigMapName); err == nil {
+	if icm, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, IstioConfigMapName(in.conf, "")); 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, cfg.ExternalServices.Istio.ConfigMapName)
+	istioConfig, err := kubeCache.GetConfigMap(cfg.IstioNamespace, IstioConfigMapName(*cfg, ""))
 	if err != nil {
 		return true
 	}