From 75180798b1d5b3b8d04852b4c72605db77b99b2f Mon Sep 17 00:00:00 2001 From: Jay Shaughnessy Date: Wed, 25 Oct 2023 16:06:02 -0400 Subject: [PATCH] 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 https://github.com/kiali/kiali/issues/6772 --- graph/telemetry/istio/appender/health_test.go | 30 +++++++++++++++---- graph/telemetry/istio/appender/mesh_check.go | 28 ++++++++--------- .../istio/appender/mesh_check_test.go | 30 ++++++++++++++----- .../istio/appender/service_entry_test.go | 28 +++++++++++------ 4 files changed, 77 insertions(+), 39 deletions(-) diff --git a/graph/telemetry/istio/appender/health_test.go b/graph/telemetry/istio/appender/health_test.go index 547eb808b0..d15c2b8329 100644 --- 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 index 3306b6ea81..5f7c1d6a60 100644 --- 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 index c45c25717e..913ad1dacf 100644 --- 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) diff --git a/graph/telemetry/istio/appender/service_entry_test.go b/graph/telemetry/istio/appender/service_entry_test.go index d8052c9ce1..e53f9071af 100644 --- a/graph/telemetry/istio/appender/service_entry_test.go +++ b/graph/telemetry/istio/appender/service_entry_test.go @@ -1,6 +1,7 @@ package appender import ( + "fmt" "testing" "time" @@ -186,11 +187,12 @@ func TestServiceEntry(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace", @@ -303,11 +305,12 @@ func TestServiceEntryExportAll(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace", @@ -414,11 +417,12 @@ func TestServiceEntryExportNamespaceFound(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace", @@ -525,11 +529,12 @@ func TestServiceEntryExportDefinitionNamespace(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace", @@ -636,11 +641,12 @@ func TestServiceEntryExportNamespaceNotFound(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace", @@ -731,10 +737,11 @@ func TestDisjointMulticlusterEntries(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("namespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "namespace", @@ -871,11 +878,12 @@ func TestServiceEntrySameHostMatchNamespace(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace", @@ -999,11 +1007,12 @@ func TestServiceEntrySameHostNoMatchNamespace(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("otherNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "otherNamespace", @@ -1108,11 +1117,12 @@ func TestServiceEntryMultipleEdges(t *testing.T) { globalInfo := graph.NewAppenderGlobalInfo() globalInfo.Business = businessLayer namespaceInfo := graph.NewAppenderNamespaceInfo("testNamespace") + key := fmt.Sprintf("%s:%s", config.DefaultClusterID, "testNamespace") // Run the appender... a := ServiceEntryAppender{ AccessibleNamespaces: graph.AccessibleNamespaces{ - config.DefaultClusterID: &graph.AccessibleNamespace{ + key: &graph.AccessibleNamespace{ Cluster: config.DefaultClusterID, CreationTimestamp: time.Now(), Name: "testNamespace",