Make sure mesh_check appender only tries to check nodes in accessible…

… namespaces. Otherwise the fetch of workload information will fail. - Fix tests - update some incorrectly defined AccessibleNamespace structures - ensure config properly defines cluster kiali#6772
jshaughn · Oct 25, 2023 · 7518079 · 7518079
1 parent 2f67e24
commit 7518079
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 39 deletions.
diff --git a/graph/telemetry/istio/appender/health_test.go b/graph/telemetry/istio/appender/health_test.go
@@ -29,7 +29,9 @@ const (
 )
 
 func TestServicesHealthConfigPasses(t *testing.T) {
-	config.Set(config.NewConfig())
+	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	trafficMap := buildServiceTrafficMap()
 	businessLayer := setupHealthConfig(t, buildFakeServicesHealth(rateDefinition), buildFakeWorkloadDeploymentsHealth(rateWorkloadDefinition), buildFakePodsHealth(rateWorkloadDefinition))
 
@@ -206,7 +208,9 @@ func TestHealthDataPresent200SvcWk(t *testing.T) {
 func TestHealthDataPresent200500WkSvc(t *testing.T) {
 	assert := assert.New(t)
 
-	config.Set(config.NewConfig())
+	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	svcNodes := buildServiceTrafficMap()
 	appNodes := buildAppTrafficMap()
 	wkNodes := buildWorkloadTrafficMap()
@@ -264,7 +268,9 @@ func TestHealthDataPresent200500WkSvc(t *testing.T) {
 func TestHealthDataPresentToApp(t *testing.T) {
 	assert := assert.New(t)
 
-	config.Set(config.NewConfig())
+	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	svcNodes := buildServiceTrafficMap()
 	appNodes := buildAppTrafficMap()
 	wkNodes := buildWorkloadTrafficMap()
@@ -316,7 +322,9 @@ func TestHealthDataPresentToApp(t *testing.T) {
 func TestHealthDataPresentFromApp(t *testing.T) {
 	assert := assert.New(t)
 
-	config.Set(config.NewConfig())
+	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	svcNodes := buildServiceTrafficMap()
 	appNodes := buildAppTrafficMap()
 	wkNodes := buildWorkloadTrafficMap()
@@ -372,7 +380,9 @@ func TestHealthDataPresentFromApp(t *testing.T) {
 func TestHealthDataBadResponses(t *testing.T) {
 	assert := assert.New(t)
 
-	config.Set(config.NewConfig())
+	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	svcNodes := buildServiceTrafficMap()
 	appNodes := buildAppTrafficMap()
 	wkNodes := buildWorkloadTrafficMap()
@@ -428,7 +438,9 @@ func TestHealthDataBadResponses(t *testing.T) {
 func TestIdleNodesHaveHealthData(t *testing.T) {
 	assert := assert.New(t)
 
-	config.Set(config.NewConfig())
+	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	trafficMap := make(graph.TrafficMap)
 	idleNode, _ := graph.NewNode("cluster-default", "testNamespace", "svc", "", "", "", "v1", graph.GraphTypeVersionedApp)
 	trafficMap[idleNode.ID] = idleNode
@@ -478,6 +490,8 @@ func TestErrorCausesPanic(t *testing.T) {
 	var k8s kubernetes.ClientInterface = kubetest.NewFakeK8sClient(objects...)
 
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	conf.ExternalServices.Istio.IstioAPIEnabled = false
 	config.Set(conf)
 	cache := business.NewTestingCache(t, k8s, *conf)
@@ -534,6 +548,8 @@ func TestMultiClusterHealthConfig(t *testing.T) {
 	}
 
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	conf.ExternalServices.Istio.IstioAPIEnabled = false
 	conf.KubernetesConfig.ClusterName = "east"
 	config.Set(conf)
@@ -610,6 +626,8 @@ func setupHealthConfig(t *testing.T, services []core_v1.Service, deployments []a
 	k8s := kubetest.NewFakeK8sClient(objects...)
 
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
+	config.Set(conf)
 	conf.ExternalServices.Istio.IstioAPIEnabled = false
 	config.Set(conf)
 	business.SetupBusinessLayer(t, k8s, *conf)

diff --git a/graph/telemetry/istio/appender/mesh_check.go b/graph/telemetry/istio/appender/mesh_check.go
@@ -1,6 +1,8 @@
 package appender
 
 import (
+	"fmt"
+
 	"github.com/kiali/kiali/config"
 	"github.com/kiali/kiali/graph"
 )
@@ -36,15 +38,15 @@ func (a MeshCheckAppender) AppendGraph(trafficMap graph.TrafficMap, globalInfo *
 
 func (a *MeshCheckAppender) applyMeshChecks(trafficMap graph.TrafficMap, globalInfo *graph.AppenderGlobalInfo, namespaceInfo *graph.AppenderNamespaceInfo) {
 	for _, n := range trafficMap {
-		// skip if we already determined there is a missing sidecar. we can process the same
-		// node multiple times because to ensure we check every node (missing sidecars indicate missing
-		// telemetry so we need to check nodes when we can, regardless of namespace)
+		// skip if we've already determined the node is out-of-mesh. we may process the same
+		// node multiple times to ensure we check every node (e.g. missing sidecars indicate missing
+		// telemetry and so we need to check nodes when we can, regardless of namespace)
 		if n.Metadata[graph.IsOutOfMesh] == true {
 			continue
 		}
 
-		// skip if the node's namespace is outside of the accessible namespaces
-		if !a.namespaceOK(n.Namespace, namespaceInfo) {
+		// skip if the node is not in an accessible namespace, we can't do the checking
+		if !a.nodeOK(n, namespaceInfo) {
 			continue
 		}
 
@@ -95,15 +97,9 @@ func (a *MeshCheckAppender) applyMeshChecks(trafficMap graph.TrafficMap, globalI
 	}
 }
 
-// namespaceOk returns true if the namespace in question is the current appender namespace or any of the graph namespaces
-func (a *MeshCheckAppender) namespaceOK(namespace string, namespaceInfo *graph.AppenderNamespaceInfo) bool {
-	if namespace == namespaceInfo.Namespace {
-		return true
-	}
-	for _, ns := range a.AccessibleNamespaces {
-		if namespace == ns.Name {
-			return true
-		}
-	}
-	return false
+// nodeOK returns true if we have access to its workload info
+func (a *MeshCheckAppender) nodeOK(node *graph.Node, namespaceInfo *graph.AppenderNamespaceInfo) bool {
+	key := fmt.Sprintf("%s:%s", node.Cluster, node.Namespace)
+	_, ok := a.AccessibleNamespaces[key]
+	return ok
 }
diff --git a/graph/telemetry/istio/appender/mesh_check_test.go b/graph/telemetry/istio/appender/mesh_check_test.go
@@ -1,6 +1,7 @@
 package appender
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -24,10 +25,11 @@ func TestWorkloadSidecarsPasses(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -47,10 +49,11 @@ func TestWorkloadWithMissingSidecarsIsFlagged(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -71,10 +74,11 @@ func TestInaccessibleWorkload(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -94,10 +98,11 @@ func TestAppNoPodsPasses(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -117,10 +122,11 @@ func TestAppSidecarsPasses(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -140,10 +146,11 @@ func TestAppWithMissingSidecarsIsFlagged(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -164,10 +171,11 @@ func TestAppWithAmbientIsFlagged(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -188,10 +196,11 @@ func TestServicesAreAlwaysValid(t *testing.T) {
 	globalInfo := graph.NewAppenderGlobalInfo()
 	globalInfo.Business = businessLayer
 	namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace")
+	key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace")
 
 	a := MeshCheckAppender{
 		AccessibleNamespaces: map[string]*graph.AccessibleNamespace{
-			config.DefaultClusterID: &graph.AccessibleNamespace{
+			key: &graph.AccessibleNamespace{
 				Cluster:           config.DefaultClusterID,
 				CreationTimestamp: time.Now(),
 				Name:              "testNamespace",
@@ -207,6 +216,7 @@ func TestServicesAreAlwaysValid(t *testing.T) {
 func buildWorkloadTrafficMap() graph.TrafficMap {
 	trafficMap := graph.NewTrafficMap()
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
 	config.Set(conf)
 	node, _ := graph.NewNode(conf.KubernetesConfig.ClusterName, "testNamespace", "", "testNamespace", "workload-1", graph.Unknown, graph.Unknown, graph.GraphTypeWorkload)
 	trafficMap[node.ID] = node
@@ -217,6 +227,7 @@ func buildWorkloadTrafficMap() graph.TrafficMap {
 func buildInaccessibleWorkloadTrafficMap() graph.TrafficMap {
 	trafficMap := graph.NewTrafficMap()
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
 	config.Set(conf)
 	node, _ := graph.NewNode(conf.KubernetesConfig.ClusterName, "inaccessibleNamespace", "", "inaccessibleNamespace", "workload-1", graph.Unknown, graph.Unknown, graph.GraphTypeVersionedApp)
 	trafficMap[node.ID] = node
@@ -227,6 +238,7 @@ func buildInaccessibleWorkloadTrafficMap() graph.TrafficMap {
 func buildAppTrafficMap() graph.TrafficMap {
 	trafficMap := graph.NewTrafficMap()
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
 	config.Set(conf)
 	node, _ := graph.NewNode(conf.KubernetesConfig.ClusterName, "testNamespace", "", "testNamespace", graph.Unknown, "myTest", graph.Unknown, graph.GraphTypeVersionedApp)
 	trafficMap[node.ID] = node
@@ -237,6 +249,7 @@ func buildAppTrafficMap() graph.TrafficMap {
 func buildServiceTrafficMap() graph.TrafficMap {
 	trafficMap := graph.NewTrafficMap()
 	conf := config.NewConfig()
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
 	config.Set(conf)
 	node, _ := graph.NewNode(conf.KubernetesConfig.ClusterName, "testNamespace", "svc", "testNamespace", graph.Unknown, graph.Unknown, graph.Unknown, graph.GraphTypeVersionedApp)
 	trafficMap[node.ID] = node
@@ -311,6 +324,7 @@ func setupSidecarsCheckWorkloads(t *testing.T, deployments []apps_v1.Deployment,
 
 	conf := config.NewConfig()
 	conf.ExternalServices.Istio.IstioAPIEnabled = false
+	conf.KubernetesConfig.ClusterName = config.DefaultClusterID
 	config.Set(conf)
 
 	business.SetupBusinessLayer(t, k8s, *conf)