KIALI-1698 Make VS and CB badging consistent

Here are the rules applied:
- VS Badging is done only on service nodes in the graph.
- CB Badging is done on a Service node with a CB destination rule or a subset with a CB destination rule
  - note: CB dest rule = traffic policy of outlierDetection or connectionPool
- CB Badging is done on a destination App node handling a CB-service.
- CB Badging is done on a destination VersionedApp node handling a CB-service with CB destination rule subset relevant to the version.
- CB Badging is not done on a App Box containing a CB-badged node. CB Badging is never done on an App Box. It is too noisy as the relevant member node will already be badged.

graph/appender/istio_details.go

@@ -36,35 +36,53 @@ func addRouteBadges(trafficMap graph.TrafficMap, namespace string, istioDetails
 func applyCircuitBreakers(trafficMap graph.TrafficMap, namespace string, istioDetails *kubernetes.IstioDetails) {
 NODES:
 	for _, n := range trafficMap {
-		version := n.Version
-		if version == graph.UnknownVersion {
-			version = ""
-		}
-
-		if destServices, ok := n.Metadata["destServices"]; ok {
-			for serviceName, _ := range destServices.(map[string]bool) {
-				for _, destinationRule := range istioDetails.DestinationRules {
-					if kubernetes.CheckDestinationRuleCircuitBreaker(destinationRule, namespace, serviceName, version) {
-						n.Metadata["hasCB"] = true
-						continue NODES
+		versionOk := n.Version != "" && n.Version != graph.UnknownVersion
+		switch {
+		case n.NodeType == graph.NodeTypeService:
+			for _, destinationRule := range istioDetails.DestinationRules {
+				if kubernetes.CheckDestinationRuleCircuitBreaker(destinationRule, namespace, n.Service, "") {
+					n.Metadata["hasCB"] = true
+					continue NODES
+				}
+			}
+		case !versionOk && (n.NodeType == graph.NodeTypeApp):
+			if destServices, ok := n.Metadata["destServices"]; ok {
+				for serviceName, _ := range destServices.(map[string]bool) {
+					for _, destinationRule := range istioDetails.DestinationRules {
+						if kubernetes.CheckDestinationRuleCircuitBreaker(destinationRule, namespace, serviceName, "") {
+							n.Metadata["hasCB"] = true
+							continue NODES
+						}
+					}
+				}
+			}
+		case versionOk:
+			if destServices, ok := n.Metadata["destServices"]; ok {
+				for serviceName, _ := range destServices.(map[string]bool) {
+					for _, destinationRule := range istioDetails.DestinationRules {
+						if kubernetes.CheckDestinationRuleCircuitBreaker(destinationRule, namespace, serviceName, n.Version) {
+							n.Metadata["hasCB"] = true
+							continue NODES
+						}
 					}
 				}
 			}
+		default:
+			continue
 		}
 	}
 }
 
 func applyVirtualServices(trafficMap graph.TrafficMap, namespace string, istioDetails *kubernetes.IstioDetails) {
 NODES:
 	for _, n := range trafficMap {
-		if destServices, ok := n.Metadata["destServices"]; ok {
-			for serviceName, _ := range destServices.(map[string]bool) {
-				for _, virtualService := range istioDetails.VirtualServices {
-					if kubernetes.CheckVirtualService(virtualService, namespace, serviceName) {
-						n.Metadata["hasVS"] = true
-						continue NODES
-					}
-				}
+		if n.NodeType != graph.NodeTypeService {
+			continue
+		}
+		for _, virtualService := range istioDetails.VirtualServices {
+			if kubernetes.CheckVirtualService(virtualService, namespace, n.Service) {
+				n.Metadata["hasVS"] = true
+				continue NODES
 			}
 		}
 	}

graph/cytoscape/cytoscape.go

@@ -371,31 +371,17 @@ func groupByVersion(nodes *[]*NodeWrapper) {
 			}
 
 			// assign each app version node to the compound parent
-			hasVirtualService := false
-			nd.HasMissingSC = false
+			nd.HasMissingSC = false // TODO: this is probably unecessarily noisy
+			nd.IsInaccessible = false
 			nd.IsOutside = false
 
 			for _, n := range members {
 				n.Parent = nodeId
 
+				// copy some member attributes to to the compound node (aka app box)
 				nd.HasMissingSC = nd.HasMissingSC || n.HasMissingSC
+				nd.IsInaccessible = nd.IsInaccessible || n.IsInaccessible
 				nd.IsOutside = nd.IsOutside || n.IsOutside
-
-				// If there is a virtual service defined in version node, move it to compound parent
-				if n.HasVS {
-					n.HasVS = false
-					hasVirtualService = true
-				}
-
-				// If we have any nodes which are inaccessible, then mark the group as being inaccessible
-				// Note: all nodes here would have the same inaccessible value since they all belong to the same namespace
-				if n.IsInaccessible {
-					nd.IsInaccessible = true
-				}
-			}
-
-			if hasVirtualService {
-				nd.HasVS = true
 			}
 
 			// add the compound node to the list of nodes

kubernetes/istio_details_service.go

@@ -323,17 +323,19 @@ func CheckDestinationRuleCircuitBreaker(destinationRule IstioObject, namespace s
 	cfg := config.Get()
 	if dHost, ok := destinationRule.GetSpec()["host"]; ok {
 		if host, ok := dHost.(string); ok && FilterByHost(host, serviceName, namespace) {
+			// CB is set at DR level, so it's true for the service and all versions
 			if trafficPolicy, ok := destinationRule.GetSpec()["trafficPolicy"]; ok && checkTrafficPolicy(trafficPolicy) {
 				return true
 			}
-			if version == "" {
-				return false
-			}
 			if subsets, ok := destinationRule.GetSpec()["subsets"]; ok {
 				if dSubsets, ok := subsets.([]interface{}); ok {
 					for _, subset := range dSubsets {
 						if innerSubset, ok := subset.(map[string]interface{}); ok {
 							if trafficPolicy, ok := innerSubset["trafficPolicy"]; ok && checkTrafficPolicy(trafficPolicy) {
+								// set the service true if it has a subset with a CB
+								if "" == version {
+									return true
+								}
 								if labels, ok := innerSubset["labels"]; ok {
 									if dLabels, ok := labels.(map[string]interface{}); ok {
 										if versionValue, ok := dLabels[cfg.IstioLabels.VersionLabelName]; ok && versionValue == version {

kubernetes/istio_details_service_test.go

@@ -207,6 +207,9 @@ func TestCheckDestinationRuleCircuitBreaker(t *testing.T) {
 
 	assert.False(t, CheckDestinationRuleCircuitBreaker(nil, "", "", ""))
 
+	// Note - I don't think the subset definitions here have any impact on the CB
+	// detection. They do not do any sort of override so presumably any version, including
+	// a v3 would inherit the DR-level CB definition.
 	destinationRule1 := MockIstioObject{
 		Spec: map[string]interface{}{
 			"host": "reviews",
@@ -239,8 +242,11 @@ func TestCheckDestinationRuleCircuitBreaker(t *testing.T) {
 		},
 	}
 
+	assert.True(t, CheckDestinationRuleCircuitBreaker(&destinationRule1, "", "reviews", ""))
+	assert.False(t, CheckDestinationRuleCircuitBreaker(&destinationRule1, "", "reviews-bad", ""))
 	assert.True(t, CheckDestinationRuleCircuitBreaker(&destinationRule1, "", "reviews", "v1"))
 	assert.True(t, CheckDestinationRuleCircuitBreaker(&destinationRule1, "", "reviews", "v2"))
+	assert.True(t, CheckDestinationRuleCircuitBreaker(&destinationRule1, "", "reviews", "v3"))
 	assert.False(t, CheckDestinationRuleCircuitBreaker(&destinationRule1, "", "reviews-bad", "v2"))
 
 	destinationRule2 := MockIstioObject{
@@ -275,6 +281,7 @@ func TestCheckDestinationRuleCircuitBreaker(t *testing.T) {
 		},
 	}
 
+	assert.True(t, CheckDestinationRuleCircuitBreaker(&destinationRule2, "", "reviews", ""))
 	assert.False(t, CheckDestinationRuleCircuitBreaker(&destinationRule2, "", "reviews", "v1"))
 	assert.True(t, CheckDestinationRuleCircuitBreaker(&destinationRule2, "", "reviews", "v2"))
 	assert.False(t, CheckDestinationRuleCircuitBreaker(&destinationRule2, "", "reviews-bad", "v2"))