[1.16.x] Fix topology view when displaying mixed connect-native/norma…

…l services. (#18330) Fix topology view when displaying mixed connect-native/normal services. (#13023) * Fix topoloy intention with mixed connect-native/normal services. If a service is registered twice, once with connect-native and once without, the topology views would prune the existing intentions. This change brings the code more in line with the transparent proxy behavior. * Dedupe nodes in the ServiceTopology ui endpoint (like done with tags). * Consider a service connect-native as soon as one instance is. Co-authored-by: Florian Apolloner <[email protected]>
hashicorp · Jul 31, 2023 · e592cf0 · e592cf0
1 parent 02b33a6
commit e592cf0
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 11 deletions.
diff --git a/.changelog/13023.txt b/.changelog/13023.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+ui: the topology view now properly displays services with mixed connect and non-connect instances.
+```
diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go
@@ -4535,13 +4535,17 @@ func (s *Store) ServiceTopology(
 		maxIdx = idx
 	}
 
-	// Store downstreams with at least one instance in transparent proxy mode.
+	// Store downstreams with at least one instance in transparent proxy or connect native mode.
 	// This is to avoid returning downstreams from intentions when none of the downstreams are transparent proxies.
-	tproxyMap := make(map[structs.ServiceName]struct{})
+	proxyMap := make(map[structs.ServiceName]struct{})
 	for _, downstream := range unfilteredDownstreams {
 		if downstream.Service.Proxy.Mode == structs.ProxyModeTransparent {
 			sn := structs.NewServiceName(downstream.Service.Proxy.DestinationServiceName, &downstream.Service.EnterpriseMeta)
-			tproxyMap[sn] = struct{}{}
+			proxyMap[sn] = struct{}{}
+		}
+		if downstream.Service.Connect.Native {
+			sn := downstream.Service.CompoundServiceName()
+			proxyMap[sn] = struct{}{}
 		}
 	}
 
@@ -4551,7 +4555,7 @@ func (s *Store) ServiceTopology(
 		if downstream.Service.Kind == structs.ServiceKindConnectProxy {
 			sn = structs.NewServiceName(downstream.Service.Proxy.DestinationServiceName, &downstream.Service.EnterpriseMeta)
 		}
-		if _, ok := tproxyMap[sn]; !ok && !downstream.Service.Connect.Native && downstreamSources[sn.String()] != structs.TopologySourceRegistration {
+		if _, ok := proxyMap[sn]; !ok && downstreamSources[sn.String()] != structs.TopologySourceRegistration {
 			// If downstream is not a transparent proxy or connect native, remove references
 			delete(downstreamSources, sn.String())
 			delete(downstreamDecisions, sn.String())
@@ -4580,6 +4584,7 @@ func (s *Store) combinedServiceNodesTxn(tx ReadTxn, ws memdb.WatchSet, names []s
 		maxIdx uint64
 		resp   structs.CheckServiceNodes
 	)
+	dedupMap := make(map[string]structs.CheckServiceNode)
 	for _, u := range names {
 		// Collect typical then connect instances
 		idx, csn, err := checkServiceNodesTxn(tx, ws, u.Name, false, &u.EnterpriseMeta, peerName)
@@ -4589,7 +4594,9 @@ func (s *Store) combinedServiceNodesTxn(tx ReadTxn, ws memdb.WatchSet, names []s
 		if idx > maxIdx {
 			maxIdx = idx
 		}
-		resp = append(resp, csn...)
+		for _, item := range csn {
+			dedupMap[item.Node.Node+"/"+item.Service.ID] = item
+		}
 
 		idx, csn, err = checkServiceNodesTxn(tx, ws, u.Name, true, &u.EnterpriseMeta, peerName)
 		if err != nil {
@@ -4598,7 +4605,12 @@ func (s *Store) combinedServiceNodesTxn(tx ReadTxn, ws memdb.WatchSet, names []s
 		if idx > maxIdx {
 			maxIdx = idx
 		}
-		resp = append(resp, csn...)
+		for _, item := range csn {
+			dedupMap[item.Node.Node+"/"+item.Service.ID] = item
+		}
+	}
+	for _, item := range dedupMap {
+		resp = append(resp, item)
 	}
 	return maxIdx, resp, nil
 }

diff --git a/agent/ui_endpoint.go b/agent/ui_endpoint.go
@@ -564,11 +564,25 @@ func summarizeServices(dump structs.ServiceDump, cfg *config.RuntimeConfig, dc s
 		sum := getService(psn)
 
 		svc := csn.Service
-		sum.Nodes = append(sum.Nodes, csn.Node.Node)
+
+		found := false
+		for _, existing := range sum.Nodes {
+			if existing == csn.Node.Node {
+				found = true
+				break
+			}
+		}
+		if !found {
+			sum.Nodes = append(sum.Nodes, csn.Node.Node)
+		}
+
 		sum.Kind = svc.Kind
 		sum.Datacenter = csn.Node.Datacenter
 		sum.InstanceCount += 1
-		sum.ConnectNative = svc.Connect.Native
+		// Consider a service connect native once at least one instance is
+		if svc.Connect.Native {
+			sum.ConnectNative = svc.Connect.Native
+		}
 		if svc.Kind == structs.ServiceKindConnectProxy {
 			sn := structs.NewServiceName(svc.Proxy.DestinationServiceName, &svc.EnterpriseMeta)
 			psn := structs.PeeredServiceName{Peer: peerName, ServiceName: sn}

diff --git a/agent/ui_endpoint_test.go b/agent/ui_endpoint_test.go
@@ -1687,19 +1687,43 @@ func TestUIServiceTopology(t *testing.T) {
 				SkipNodeUpdate: true,
 				Service: &structs.NodeService{
 					Kind:    structs.ServiceKindTypical,
-					ID:      "cproxy",
+					ID:      "cproxy-https",
 					Service: "cproxy",
 					Port:    1111,
 					Address: "198.18.1.70",
+					Tags:    []string{"https"},
 					Connect: structs.ServiceConnect{Native: true},
 				},
 				Checks: structs.HealthChecks{
 					&structs.HealthCheck{
 						Node:        "cnative",
-						CheckID:     "cnative:cproxy",
+						CheckID:     "cnative:cproxy-https",
 						Name:        "cproxy-liveness",
 						Status:      api.HealthPassing,
-						ServiceID:   "cproxy",
+						ServiceID:   "cproxy-https",
+						ServiceName: "cproxy",
+					},
+				},
+			},
+			"Service cproxy/http on cnative": {
+				Datacenter:     "dc1",
+				Node:           "cnative",
+				SkipNodeUpdate: true,
+				Service: &structs.NodeService{
+					Kind:    structs.ServiceKindTypical,
+					ID:      "cproxy-http",
+					Service: "cproxy",
+					Port:    1112,
+					Address: "198.18.1.70",
+					Tags:    []string{"http"},
+				},
+				Checks: structs.HealthChecks{
+					&structs.HealthCheck{
+						Node:        "cnative",
+						CheckID:     "cnative:cproxy-http",
+						Name:        "cproxy-liveness",
+						Status:      api.HealthPassing,
+						ServiceID:   "cproxy-http",
 						ServiceName: "cproxy",
 					},
 				},
@@ -2125,6 +2149,42 @@ func TestUIServiceTopology(t *testing.T) {
 				FilteredByACLs: false,
 			},
 		},
+		{
+			name: "cbackend",
+			httpReq: func() *http.Request {
+				req, _ := http.NewRequest("GET", "/v1/internal/ui/service-topology/cbackend?kind=", nil)
+				return req
+			}(),
+			want: &ServiceTopology{
+				Protocol:         "http",
+				TransparentProxy: false,
+				Upstreams:        []*ServiceTopologySummary{},
+				Downstreams: []*ServiceTopologySummary{
+					{
+						ServiceSummary: ServiceSummary{
+							Name:           "cproxy",
+							Datacenter:     "dc1",
+							Tags:           []string{"http", "https"},
+							Nodes:          []string{"cnative"},
+							InstanceCount:  2,
+							ChecksPassing:  3,
+							ChecksWarning:  0,
+							ChecksCritical: 0,
+							ConnectNative:  true,
+							EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(),
+						},
+						Intention: structs.IntentionDecisionSummary{
+							DefaultAllow:   true,
+							Allowed:        true,
+							HasPermissions: false,
+							HasExact:       true,
+						},
+						Source: structs.TopologySourceSpecificIntention,
+					},
+				},
+				FilteredByACLs: false,
+			},
+		},
 	}
 
 	for _, tc := range tcs {