From cf4eee11a742ee9f922c10aeefbb49743aa54c8f Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 1 Sep 2016 21:51:20 +0100 Subject: [PATCH] fix internet connection counts For counting we were using a table keyed on a struct containing Node pointers. For connections between ordinary nodes this works just fine. But for connections to/from the Internet node we want to track individual address/port combinations, which involves an extra lookup. Since our data structures generally contain values, not pointers, this produces aliases. As a result n connections from/to a node to/from a specific Internet IP+port would result in n rows in the count table, each with a count of 1, instead of one row with a count of n. Things wouldn't be so bad if it was actually rendered like that - annoying, but at least accurate - but... Each row has an ID which is computed from the node IDs and ports. Not Node references. The ID must be unique - the frontend will only render *one* thing per ID. Since the row IDs of our n rows are all the same, we see one row with a count of 1 instead of n rows with a count of 1. Furthermore, since the frontend's table row limiting is counting rows, not unique row IDs, a) fewer rows would be rendered than expected, and b) the displayed count of the number of extra rows would be too high. The fix is to replace the Node pointers in the key with Node IDs. This does require an extra table lookup when we come to produce the rows, but that is just a fairly cheap map lookup. Fixes #1495. --- render/detailed/connections.go | 46 +++++++++++++--------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/render/detailed/connections.go b/render/detailed/connections.go index 0a4decfae..03c6c81f6 100644 --- a/render/detailed/connections.go +++ b/render/detailed/connections.go @@ -57,13 +57,13 @@ func (s connectionsByID) Less(i, j int) bool { return s[i].ID < s[j].ID } // Intermediate type used as a key to dedupe rows type connection struct { - remoteNode, localNode *report.Node - remoteAddr, localAddr string - port string // always the server-side port + remoteNodeID, localNodeID string + remoteAddr, localAddr string + port string // always the server-side port } func (row connection) ID() string { - return fmt.Sprintf("%s:%s-%s:%s-%s", row.remoteNode.ID, row.remoteAddr, row.localNode.ID, row.localAddr, row.port) + return fmt.Sprintf("%s:%s-%s:%s-%s", row.remoteNodeID, row.remoteAddr, row.localNodeID, row.localAddr, row.port) } func incomingConnectionsSummary(topologyID string, r report.Report, n report.Node, ns report.Nodes) ConnectionsSummary { @@ -75,10 +75,6 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !node.Adjacency.Contains(n.ID) { continue } - // Shallow-copy the node to obtain a different pointer - // on the remote side - remoteNode := node - // Work out what port they are talking to, and count the number of // connections to that port. // This is complicated as for internet nodes we break out individual @@ -92,13 +88,12 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod continue } key := connection{ - localNode: &n, - remoteNode: &remoteNode, - port: port, + localNodeID: n.ID, + remoteNodeID: node.ID, + port: port, } if isInternetNode(n) { - endpointNode := r.Endpoint.Nodes[localEndpointID] - key.localNode = &endpointNode + key.localNodeID = localEndpointID key.localAddr = localAddr } counts[key] = counts[key] + 1 @@ -115,7 +110,7 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod TopologyID: topologyID, Label: "Inbound", Columns: columnHeaders, - Connections: connectionRows(r, counts, isInternetNode(n)), + Connections: connectionRows(r, ns, counts, isInternetNode(n)), } } @@ -132,10 +127,6 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod remoteEndpointIDs := endpointChildIDsOf(node) - // Shallow-copy the node to obtain a different pointer - // on the remote side - remoteNode := node - for _, localEndpoint := range localEndpoints { _, localAddr, _, ok := report.ParseEndpointNodeID(localEndpoint.ID) if !ok { @@ -148,13 +139,12 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod continue } key := connection{ - localNode: &n, - remoteNode: &remoteNode, - port: port, + localNodeID: n.ID, + remoteNodeID: node.ID, + port: port, } if isInternetNode(n) { - endpointNode := r.Endpoint.Nodes[remoteEndpointID] - key.localNode = &endpointNode + key.localNodeID = remoteEndpointID key.localAddr = localAddr } counts[key] = counts[key] + 1 @@ -171,7 +161,7 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod TopologyID: topologyID, Label: "Outbound", Columns: columnHeaders, - Connections: connectionRows(r, counts, isInternetNode(n)), + Connections: connectionRows(r, ns, counts, isInternetNode(n)), } } @@ -199,13 +189,13 @@ func isInternetNode(n report.Node) bool { return n.ID == render.IncomingInternetID || n.ID == render.OutgoingInternetID } -func connectionRows(r report.Report, in map[connection]int, includeLocal bool) []Connection { +func connectionRows(r report.Report, ns report.Nodes, in map[connection]int, includeLocal bool) []Connection { output := []Connection{} for row, count := range in { // Use MakeNodeSummary to render the id and label of this node // TODO(paulbellamy): Would be cleaner if we hade just a - // MakeNodeID(*row.remoteode). As we don't need the whole summary. - summary, ok := MakeNodeSummary(r, *row.remoteNode) + // MakeNodeID(ns[row.remoteNodeID]). As we don't need the whole summary. + summary, ok := MakeNodeSummary(r, ns[row.remoteNodeID]) connection := Connection{ ID: row.ID(), NodeID: summary.ID, @@ -219,7 +209,7 @@ func connectionRows(r report.Report, in map[connection]int, includeLocal bool) [ if includeLocal { // Does localNode have a DNS record in it? label := row.localAddr - if set, ok := row.localNode.Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { + if set, ok := ns[row.localNodeID].Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { label = fmt.Sprintf("%s (%s)", set[0], label) } connection.Metadata = append(connection.Metadata,