From 6ada2e05ff8193b2c9875941ee8fa39415a9b226 Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Mon, 31 Jul 2023 14:10:55 +0200 Subject: [PATCH] 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. --- .changelog/13023.txt | 3 ++ agent/consul/state/catalog.go | 24 +++++++++---- agent/ui_endpoint.go | 18 ++++++++-- agent/ui_endpoint_test.go | 66 +++++++++++++++++++++++++++++++++-- 4 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 .changelog/13023.txt diff --git a/.changelog/13023.txt b/.changelog/13023.txt new file mode 100644 index 0000000000..cadf7bb938 --- /dev/null +++ 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 index 4e9fcf716c..040a960814 100644 --- 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 index 5924e23f6f..20d6f8a412 100644 --- a/agent/ui_endpoint.go +++ b/agent/ui_endpoint.go @@ -563,11 +563,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 index f6810db801..d2cda642f2 100644 --- 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 {