diff --git a/business/istio_certs.go b/business/istio_certs.go index aaebb3f149..a4a531b1ca 100644 --- a/business/istio_certs.go +++ b/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 } diff --git a/business/istio_validations.go b/business/istio_validations.go index 92668a3a97..ad19e444b4 100644 --- a/business/istio_validations.go +++ b/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 diff --git a/business/mesh.go b/business/mesh.go index 52dd627b31..0f0845e03b 100644 --- a/business/mesh.go +++ b/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 } diff --git a/business/mesh_internal_test.go b/business/mesh_internal_test.go new file mode 100644 index 0000000000..c3d05b44de --- /dev/null +++ b/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) + }) + } +} diff --git a/business/mesh_test.go b/business/mesh_test.go index 88220c90f5..3f7c6e9fb0 100644 --- a/business/mesh_test.go +++ b/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) - }) - } -} diff --git a/business/namespaces.go b/business/namespaces.go index 15e0a2fe7a..9bf63d3756 100644 --- a/business/namespaces.go +++ b/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 { diff --git a/business/tls.go b/business/tls.go index a58e8feed7..0bb7b736cc 100644 --- a/business/tls.go +++ b/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 }