From e009548ace0939d74c226ed7373c334cae118bbc Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 15 Sep 2016 11:45:16 +0100 Subject: [PATCH 1/4] refactor: shrink connection count key - remoteAddr was never being set - localAddr is part of the localNodeID in all the cases it was set, so it effectively was part of the key twice --- render/detailed/connections.go | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/render/detailed/connections.go b/render/detailed/connections.go index ce8deaa65..779544df8 100644 --- a/render/detailed/connections.go +++ b/render/detailed/connections.go @@ -58,11 +58,10 @@ 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 { remoteNodeID, localNodeID string - remoteAddr, localAddr string port string // always the server-side port } -func newConnection(n report.Node, node report.Node, port string, endpointID string, localAddr string) connection { +func newConnection(n report.Node, node report.Node, port string, endpointID string) connection { c := connection{ localNodeID: n.ID, remoteNodeID: node.ID, @@ -71,18 +70,16 @@ func newConnection(n report.Node, node report.Node, port string, endpointID stri // For internet nodes we break out individual addresses, both when // the internet node is remote (an incoming connection from the // internet) and 'local' (ie you are loading details on the - // internet node) + // internet node). Hence we use the *endpoint* ID here since that + // gives us the address and reverse DNS information. if isInternetNode(n) { - // We use the *endpoint* ID here since that has the reverse - // DNS information associated with it. c.localNodeID = endpointID - c.localAddr = localAddr } return c } func (row connection) ID() string { - return fmt.Sprintf("%s:%s-%s:%s-%s", row.remoteNodeID, row.remoteAddr, row.localNodeID, row.localAddr, row.port) + return fmt.Sprintf("%s-%s-%s", row.remoteNodeID, row.localNodeID, row.port) } type connectionCounters struct { @@ -94,7 +91,7 @@ func newConnectionCounters() *connectionCounters { return &connectionCounters{counted: map[string]struct{}{}, counts: map[connection]int{}} } -func (c *connectionCounters) add(sourceEndpoint report.Node, n report.Node, node report.Node, port string, endpointID string, localAddr string) { +func (c *connectionCounters) add(sourceEndpoint report.Node, n report.Node, node report.Node, port string, endpointID string) { // We identify connections by their source endpoint, pre-NAT, to // ensure we only count them once. connectionID := sourceEndpoint.ID @@ -105,7 +102,7 @@ func (c *connectionCounters) add(sourceEndpoint report.Node, n report.Node, node return } c.counted[connectionID] = struct{}{} - key := newConnection(n, node, port, endpointID, localAddr) + key := newConnection(n, node, port, endpointID) c.counts[key]++ } @@ -115,21 +112,17 @@ func (c *connectionCounters) rows(r report.Report, ns report.Nodes, includeLocal // Use MakeNodeSummary to render the id and label of this node // TODO(paulbellamy): Would be cleaner if we hade just a // MakeNodeID(ns[row.remoteNodeID]). As we don't need the whole summary. - summary, ok := MakeNodeSummary(r, ns[row.remoteNodeID]) + summary, _ := MakeNodeSummary(r, ns[row.remoteNodeID]) connection := Connection{ ID: row.ID(), NodeID: summary.ID, Label: summary.Label, Linkable: true, } - if !ok && row.remoteAddr != "" { - connection.Label = row.remoteAddr - connection.Linkable = false - } if includeLocal { + _, label, _, _ := report.ParseEndpointNodeID(row.localNodeID) // Does localNode (which, in this case, is an endpoint) // have a DNS record in it? - label := row.localAddr if set, ok := r.Endpoint.Nodes[row.localNodeID].Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { label = fmt.Sprintf("%s (%s)", set[0], label) } @@ -172,11 +165,11 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod for _, remoteEndpoint := range endpointChildrenOf(node) { for _, localEndpointID := range remoteEndpoint.Adjacency.Intersection(localEndpointIDs) { localEndpointID = canonicalEndpointID(localEndpointIDCopies, localEndpointID) - _, localAddr, port, ok := report.ParseEndpointNodeID(localEndpointID) + _, _, port, ok := report.ParseEndpointNodeID(localEndpointID) if !ok { continue } - counts.add(remoteEndpoint, n, node, port, localEndpointID, localAddr) + counts.add(remoteEndpoint, n, node, port, localEndpointID) } } } @@ -208,17 +201,13 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod remoteEndpointIDs, remoteEndpointIDCopies := endpointChildIDsAndCopyMapOf(node) for _, localEndpoint := range localEndpoints { - _, localAddr, _, ok := report.ParseEndpointNodeID(localEndpoint.ID) - if !ok { - continue - } for _, remoteEndpointID := range localEndpoint.Adjacency.Intersection(remoteEndpointIDs) { remoteEndpointID = canonicalEndpointID(remoteEndpointIDCopies, remoteEndpointID) _, _, port, ok := report.ParseEndpointNodeID(remoteEndpointID) if !ok { continue } - counts.add(localEndpoint, n, node, port, localEndpoint.ID, localAddr) + counts.add(localEndpoint, n, node, port, localEndpoint.ID) } } } From 0e1aeec6ae95e9245c5852a5f881f6673467a95d Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 15 Sep 2016 15:03:44 +0100 Subject: [PATCH 2/4] key internet node connection counts differently Instead of including the localEndpointID in the key, use the address, so the port is excluded. That way we don't end up with multiple rows for inbound connections from the same internet address. We also augment the address with the DNS info here, which saves us from having to look it up later on. Fixes #1867 --- render/detailed/connections.go | 63 ++++++++++++++-------------------- 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/render/detailed/connections.go b/render/detailed/connections.go index 779544df8..46ada222d 100644 --- a/render/detailed/connections.go +++ b/render/detailed/connections.go @@ -57,29 +57,30 @@ 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 { - remoteNodeID, localNodeID string - port string // always the server-side port + remoteNodeID string + addr string // for internet nodes only: the address + port string // destination port } -func newConnection(n report.Node, node report.Node, port string, endpointID string) connection { - c := connection{ - localNodeID: n.ID, - remoteNodeID: node.ID, - port: port, +func newConnection(outgoing bool, localNode, remoteNode, localEndpoint, remoteEndpoint report.Node) connection { + c := connection{remoteNodeID: remoteNode.ID} + dstEndpointID := localEndpoint.ID + if outgoing { + dstEndpointID = remoteEndpoint.ID } - // For internet nodes we break out individual addresses, both when - // the internet node is remote (an incoming connection from the - // internet) and 'local' (ie you are loading details on the - // internet node). Hence we use the *endpoint* ID here since that - // gives us the address and reverse DNS information. - if isInternetNode(n) { - c.localNodeID = endpointID + _, _, c.port, _ = report.ParseEndpointNodeID(dstEndpointID) + // For internet nodes we break out individual addresses + if isInternetNode(localNode) { + _, c.addr, _, _ = report.ParseEndpointNodeID(localEndpoint.ID) + if set, ok := localEndpoint.Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { + c.addr = fmt.Sprintf("%s (%s)", set[0], c.addr) + } } return c } func (row connection) ID() string { - return fmt.Sprintf("%s-%s-%s", row.remoteNodeID, row.localNodeID, row.port) + return fmt.Sprintf("%s-%s-%s", row.remoteNodeID, row.addr, row.port) } type connectionCounters struct { @@ -91,9 +92,13 @@ func newConnectionCounters() *connectionCounters { return &connectionCounters{counted: map[string]struct{}{}, counts: map[connection]int{}} } -func (c *connectionCounters) add(sourceEndpoint report.Node, n report.Node, node report.Node, port string, endpointID string) { +func (c *connectionCounters) add(outgoing bool, localNode, remoteNode, localEndpoint, remoteEndpoint report.Node) { // We identify connections by their source endpoint, pre-NAT, to // ensure we only count them once. + sourceEndpoint := remoteEndpoint + if outgoing { + sourceEndpoint = localEndpoint + } connectionID := sourceEndpoint.ID if copySourceEndpointID, _, ok := sourceEndpoint.Latest.LookupEntry("copy_of"); ok { connectionID = copySourceEndpointID @@ -102,7 +107,7 @@ func (c *connectionCounters) add(sourceEndpoint report.Node, n report.Node, node return } c.counted[connectionID] = struct{}{} - key := newConnection(n, node, port, endpointID) + key := newConnection(outgoing, localNode, remoteNode, localEndpoint, remoteEndpoint) c.counts[key]++ } @@ -120,16 +125,10 @@ func (c *connectionCounters) rows(r report.Report, ns report.Nodes, includeLocal Linkable: true, } if includeLocal { - _, label, _, _ := report.ParseEndpointNodeID(row.localNodeID) - // Does localNode (which, in this case, is an endpoint) - // have a DNS record in it? - if set, ok := r.Endpoint.Nodes[row.localNodeID].Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { - label = fmt.Sprintf("%s (%s)", set[0], label) - } connection.Metadata = append(connection.Metadata, report.MetadataRow{ ID: "foo", - Value: label, + Value: row.addr, Datatype: number, }) } @@ -160,16 +159,10 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !node.Adjacency.Contains(n.ID) { continue } - // Work out what port they are talking to, and count the number of - // connections to that port. for _, remoteEndpoint := range endpointChildrenOf(node) { for _, localEndpointID := range remoteEndpoint.Adjacency.Intersection(localEndpointIDs) { localEndpointID = canonicalEndpointID(localEndpointIDCopies, localEndpointID) - _, _, port, ok := report.ParseEndpointNodeID(localEndpointID) - if !ok { - continue - } - counts.add(remoteEndpoint, n, node, port, localEndpointID) + counts.add(false, n, node, r.Endpoint.Nodes[localEndpointID], remoteEndpoint) } } } @@ -197,17 +190,11 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !ok { continue } - remoteEndpointIDs, remoteEndpointIDCopies := endpointChildIDsAndCopyMapOf(node) - for _, localEndpoint := range localEndpoints { for _, remoteEndpointID := range localEndpoint.Adjacency.Intersection(remoteEndpointIDs) { remoteEndpointID = canonicalEndpointID(remoteEndpointIDCopies, remoteEndpointID) - _, _, port, ok := report.ParseEndpointNodeID(remoteEndpointID) - if !ok { - continue - } - counts.add(localEndpoint, n, node, port, localEndpoint.ID) + counts.add(true, n, node, localEndpoint, r.Endpoint.Nodes[remoteEndpointID]) } } } From 463681dcdde8fb6590b20f0203c9cb1871c2bf85 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 15 Sep 2016 16:04:44 +0100 Subject: [PATCH 3/4] refactor: inline --- render/detailed/connections.go | 52 +++++++++++++++------------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/render/detailed/connections.go b/render/detailed/connections.go index 46ada222d..fd7528461 100644 --- a/render/detailed/connections.go +++ b/render/detailed/connections.go @@ -62,27 +62,6 @@ type connection struct { port string // destination port } -func newConnection(outgoing bool, localNode, remoteNode, localEndpoint, remoteEndpoint report.Node) connection { - c := connection{remoteNodeID: remoteNode.ID} - dstEndpointID := localEndpoint.ID - if outgoing { - dstEndpointID = remoteEndpoint.ID - } - _, _, c.port, _ = report.ParseEndpointNodeID(dstEndpointID) - // For internet nodes we break out individual addresses - if isInternetNode(localNode) { - _, c.addr, _, _ = report.ParseEndpointNodeID(localEndpoint.ID) - if set, ok := localEndpoint.Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { - c.addr = fmt.Sprintf("%s (%s)", set[0], c.addr) - } - } - return c -} - -func (row connection) ID() string { - return fmt.Sprintf("%s-%s-%s", row.remoteNodeID, row.addr, row.port) -} - type connectionCounters struct { counted map[string]struct{} counts map[connection]int @@ -95,20 +74,35 @@ func newConnectionCounters() *connectionCounters { func (c *connectionCounters) add(outgoing bool, localNode, remoteNode, localEndpoint, remoteEndpoint report.Node) { // We identify connections by their source endpoint, pre-NAT, to // ensure we only count them once. - sourceEndpoint := remoteEndpoint + srcEndpoint, dstEndpoint := remoteEndpoint, localEndpoint if outgoing { - sourceEndpoint = localEndpoint + srcEndpoint, dstEndpoint = localEndpoint, remoteEndpoint } - connectionID := sourceEndpoint.ID - if copySourceEndpointID, _, ok := sourceEndpoint.Latest.LookupEntry("copy_of"); ok { - connectionID = copySourceEndpointID + connectionID := srcEndpoint.ID + if copySrcEndpointID, _, ok := srcEndpoint.Latest.LookupEntry("copy_of"); ok { + connectionID = copySrcEndpointID } if _, ok := c.counted[connectionID]; ok { return } + + conn := connection{remoteNodeID: remoteNode.ID} + var ok bool + if _, _, conn.port, ok = report.ParseEndpointNodeID(dstEndpoint.ID); !ok { + return + } + // For internet nodes we break out individual addresses + if isInternetNode(localNode) { + if _, conn.addr, _, ok = report.ParseEndpointNodeID(localEndpoint.ID); !ok { + return + } + if set, ok := localEndpoint.Sets.Lookup(endpoint.ReverseDNSNames); ok && len(set) > 0 { + conn.addr = fmt.Sprintf("%s (%s)", set[0], conn.addr) + } + } + c.counted[connectionID] = struct{}{} - key := newConnection(outgoing, localNode, remoteNode, localEndpoint, remoteEndpoint) - c.counts[key]++ + c.counts[conn]++ } func (c *connectionCounters) rows(r report.Report, ns report.Nodes, includeLocal bool) []Connection { @@ -119,7 +113,7 @@ func (c *connectionCounters) rows(r report.Report, ns report.Nodes, includeLocal // MakeNodeID(ns[row.remoteNodeID]). As we don't need the whole summary. summary, _ := MakeNodeSummary(r, ns[row.remoteNodeID]) connection := Connection{ - ID: row.ID(), + ID: fmt.Sprintf("%s-%s-%s", row.remoteNodeID, row.addr, row.port), NodeID: summary.ID, Label: summary.Label, Linkable: true, From 591e72e702405ff40fdb66c7cda28341ac565bee Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 15 Sep 2016 17:30:43 +0100 Subject: [PATCH 4/4] fix unit tests --- render/detailed/node_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/render/detailed/node_test.go b/render/detailed/node_test.go index 9c2d34b61..aff45bc47 100644 --- a/render/detailed/node_test.go +++ b/render/detailed/node_test.go @@ -25,6 +25,10 @@ func child(t *testing.T, r render.Renderer, id string) detailed.NodeSummary { return s.SummarizeMetrics() } +func connectionID(nodeID string) string { + return fmt.Sprintf("%s-%s-%d", nodeID, "", 80) +} + func TestMakeDetailedHostNode(t *testing.T) { renderableNodes := render.HostRenderer.Render(fixture.Report, nil) renderableNode := renderableNodes[fixture.ClientHostNodeID] @@ -149,7 +153,7 @@ func TestMakeDetailedHostNode(t *testing.T) { Columns: detailed.NormalColumns, Connections: []detailed.Connection{ { - ID: fmt.Sprintf("%s:%s-%s:%s-%d", fixture.ServerHostNodeID, "", fixture.ClientHostNodeID, "", 80), + ID: connectionID(fixture.ServerHostNodeID), NodeID: fixture.ServerHostNodeID, Label: "server", Linkable: true, @@ -257,7 +261,7 @@ func TestMakeDetailedContainerNode(t *testing.T) { Columns: detailed.NormalColumns, Connections: []detailed.Connection{ { - ID: fmt.Sprintf("%s:%s-%s:%s-%d", fixture.ClientContainerNodeID, "", fixture.ServerContainerNodeID, "", 80), + ID: connectionID(fixture.ClientContainerNodeID), NodeID: fixture.ClientContainerNodeID, Label: "client", Linkable: true, @@ -275,7 +279,7 @@ func TestMakeDetailedContainerNode(t *testing.T) { }, }, { - ID: fmt.Sprintf("%s:%s-%s:%s-%d", render.IncomingInternetID, "", fixture.ServerContainerNodeID, "", 80), + ID: connectionID(render.IncomingInternetID), NodeID: render.IncomingInternetID, Label: render.InboundMajor, Linkable: true, @@ -377,7 +381,7 @@ func TestMakeDetailedPodNode(t *testing.T) { Columns: detailed.NormalColumns, Connections: []detailed.Connection{ { - ID: fmt.Sprintf("%s:%s-%s:%s-%d", fixture.ClientPodNodeID, "", fixture.ServerPodNodeID, "", 80), + ID: connectionID(fixture.ClientPodNodeID), NodeID: fixture.ClientPodNodeID, Label: "pong-a", Linkable: true, @@ -395,7 +399,7 @@ func TestMakeDetailedPodNode(t *testing.T) { }, }, { - ID: fmt.Sprintf("%s:%s-%s:%s-%d", render.IncomingInternetID, "", fixture.ServerPodNodeID, "", 80), + ID: connectionID(render.IncomingInternetID), NodeID: render.IncomingInternetID, Label: render.InboundMajor, Linkable: true,