From cd97708af0c4adf74cd264a9742bb4787b5879b8 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Tue, 20 Oct 2015 16:38:58 +0000 Subject: [PATCH] Review feedback --- probe/docker/container.go | 5 ++--- probe/docker/container_linux_test.go | 2 +- probe/overlay/weave.go | 2 +- probe/overlay/weave_test.go | 2 +- render/mapping.go | 27 +++++++++++++++++---------- report/id.go | 12 ++++++++++++ 6 files changed, 34 insertions(+), 16 deletions(-) diff --git a/probe/docker/container.go b/probe/docker/container.go index 1ca558b6d..8f78d34bb 100644 --- a/probe/docker/container.go +++ b/probe/docker/container.go @@ -224,12 +224,11 @@ func (c *container) GetNode(hostID string, localAddrs []net.IP) report.Node { c.RLock() defer c.RUnlock() - ips := append(c.container.NetworkSettings.SecondaryIPAddresses, - c.container.NetworkSettings.IPAddress) + ips := append(c.container.NetworkSettings.SecondaryIPAddresses, c.container.NetworkSettings.IPAddress) // Treat all Docker IPs as local scoped. ipsWithScopes := []string{} for _, ip := range ips { - ipsWithScopes = append(ipsWithScopes, fmt.Sprintf("%s:%s", hostID, ip)) + ipsWithScopes = append(ipsWithScopes, report.MakeScopedAddressNodeID(hostID, ip)) } result := report.MakeNodeWith(map[string]string{ diff --git a/probe/docker/container_linux_test.go b/probe/docker/container_linux_test.go index 2ceec3fe5..39f11b994 100644 --- a/probe/docker/container_linux_test.go +++ b/probe/docker/container_linux_test.go @@ -70,7 +70,7 @@ func TestContainer(t *testing.T) { "docker_container_created": "01 Jan 01 00:00 UTC", "docker_container_id": "ping", "docker_container_ips": "1.2.3.4", - "docker_container_ips_with_scopes": "scope:1.2.3.4", + "docker_container_ips_with_scopes": "scope;1.2.3.4", "docker_container_name": "pong", "docker_container_ports": "1.2.3.4:80->80/tcp, 81/tcp", "docker_image_id": "baz", diff --git a/probe/overlay/weave.go b/probe/overlay/weave.go index 92f256745..6c4c87ae4 100644 --- a/probe/overlay/weave.go +++ b/probe/overlay/weave.go @@ -181,7 +181,7 @@ func (w *Weave) Tag(r report.Report) (report.Report, error) { existingIPsWithScopes := report.MakeIDList(docker.ExtractContainerIPsWithScopes(node)...) for _, ip := range e.ips { - existingIPsWithScopes = existingIPsWithScopes.Add(fmt.Sprintf(":%s", ip)) + existingIPsWithScopes = existingIPsWithScopes.Add(report.MakeAddressNodeID("", ip)) } node.Metadata[docker.ContainerIPsWithScopes] = strings.Join(existingIPsWithScopes, " ") diff --git a/probe/overlay/weave_test.go b/probe/overlay/weave_test.go index 2ce51b28d..3950653d8 100644 --- a/probe/overlay/weave_test.go +++ b/probe/overlay/weave_test.go @@ -85,7 +85,7 @@ const ( mockContainerID = "83183a667c01" mockContainerMAC = "d6:f2:5a:12:36:a8" mockContainerIP = "10.0.0.123" - mockContainerIPWithScope = ":10.0.0.123" + mockContainerIPWithScope = ";10.0.0.123" mockHostname = "hostname.weave.local" ) diff --git a/render/mapping.go b/render/mapping.go index 2ca5a247d..3fead832d 100644 --- a/render/mapping.go +++ b/render/mapping.go @@ -291,10 +291,13 @@ func MapEndpoint2IP(m RenderableNode, local report.Networks) RenderableNodes { return RenderableNodes{TheInternetID: newDerivedPseudoNode(TheInternetID, TheInternetMajor, m)} } - // Emit 2 nodes: scope:addr and scope:addr:port, so connections from the - // internet to containers via port mapping also works. - id := fmt.Sprintf("%s:%s", scope, addr) - idWithPort := fmt.Sprintf("%s:%s:%s", scope, addr, port) + // We don't always know what port a container is listening on, and + // container-to-container communications can be unambiguously identified + // without ports. OTOH, connections to the host IPs which have been port + // mapped to a container can only be unambiguously identified with the port. + // So we need to emit two nodes, for two different cases. + id := report.MakeScopedEndpointNodeID(scope, addr, "") + idWithPort := report.MakeScopedEndpointNodeID(scope, addr, port) return RenderableNodes{ id: NewRenderableNodeWith(id, "", "", "", m), idWithPort: NewRenderableNodeWith(idWithPort, "", "", "", m), @@ -310,18 +313,22 @@ func MapContainer2IP(m RenderableNode, _ report.Networks) RenderableNodes { result := RenderableNodes{} if addrs, ok := m.Metadata[docker.ContainerIPsWithScopes]; ok { for _, addr := range strings.Fields(addrs) { - node := NewRenderableNodeWith(addr, "", "", "", m) + scope, addr, ok := report.ParseAddressNodeID(addr) + if !ok { + continue + } + id := report.MakeScopedEndpointNodeID(scope, addr, "") + node := NewRenderableNodeWith(id, "", "", "", m) node.Counters[containersKey] = 1 - result[addr] = node + result[id] = node } } - // Also output all the host:port port mappings. - // In this case, we assume this doesn't need a scope, - // as they are for host IPs. + // Also output all the host:port port mappings (see above comment). + // In this case we assume this doesn't need a scope, as they are for host IPs. for _, mapping := range portMappingMatch.FindAllStringSubmatch(m.Metadata[docker.ContainerPorts], -1) { ip, port := mapping[1], mapping[2] - id := fmt.Sprintf(":%s:%s", ip, port) + id := report.MakeScopedEndpointNodeID("", ip, port) node := NewRenderableNodeWith(id, "", "", "", m) node.Counters[containersKey] = 1 result[id] = node diff --git a/report/id.go b/report/id.go index daa816ce7..42b383611 100644 --- a/report/id.go +++ b/report/id.go @@ -76,6 +76,18 @@ func MakeAddressNodeID(hostID, address string) string { return scope + ScopeDelim + address } +// MakeScopedEndpointNodeID is like MakeEndpointNodeID, but it always +// prefixes the ID witha scope. +func MakeScopedEndpointNodeID(hostID, address, port string) string { + return hostID + ScopeDelim + address + ScopeDelim + port +} + +// MakeScopedAddressNodeID is like MakeAddressNodeID, but it always +// prefixes the ID witha scope. +func MakeScopedAddressNodeID(hostID, address string) string { + return hostID + ScopeDelim + address +} + // MakeProcessNodeID produces a process node ID from its composite parts. func MakeProcessNodeID(hostID, pid string) string { return hostID + ScopeDelim + pid