From 1f825c52ff966c110d3a6efbb60982c9045679e4 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Fri, 29 Apr 2016 04:30:58 +0000 Subject: [PATCH 1/6] Correctly attribute DNAT-ed short-lived connections --- probe/endpoint/reporter.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/probe/endpoint/reporter.go b/probe/endpoint/reporter.go index d16e72a33..c1df1eedf 100644 --- a/probe/endpoint/reporter.go +++ b/probe/endpoint/reporter.go @@ -106,7 +106,7 @@ func (r *Reporter) Report() (report.Report, error) { rpt := report.MakeReport() seenTuples := map[string]fourTuple{} - // Consult the flowWalker for short-live connections + // Consult the flowWalker for short-lived connections { extraNodeInfo := map[string]string{ Conntracked: "true", @@ -120,6 +120,25 @@ func (r *Reporter) Report() (report.Report, error) { } seenTuples[tuple.key()] = tuple r.addConnection(&rpt, tuple, extraNodeInfo, extraNodeInfo) + + // Handle DNAT-ed short-lived connections. + // The NAT mapper won't help since it only runs periodically, + // missing the short-lived connections. + if f.Original.Layer3.DstIP != f.Reply.Layer3.SrcIP { + reply_tuple := fourTuple{ + f.Reply.Layer3.DstIP, + f.Reply.Layer3.SrcIP, + uint16(f.Reply.Layer4.DstPort), + uint16(f.Reply.Layer4.SrcPort), + } + // FIXME: For DNAT-ed connections the + // f.Original.Layer3.SrcIP -> f.Original.Layer3.DstIP connection above + // results in a unattributed dangling-edge to the Internet node. + // It could be enough to simply not add that connection, but + // it may not be correct in general. + r.addConnection(&rpt, reply_tuple, extraNodeInfo, extraNodeInfo) + } + }) } From b1836acb62d95e3aad19a84dad2e98f070103b27 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 3 May 2016 15:10:27 +0000 Subject: [PATCH 2/6] Only add DNAT'ed addresses for short-lived connections --- probe/endpoint/nat_internal_test.go | 2 +- probe/endpoint/reporter.go | 13 +++---------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/probe/endpoint/nat_internal_test.go b/probe/endpoint/nat_internal_test.go index db4c0f578..29906d9a0 100644 --- a/probe/endpoint/nat_internal_test.go +++ b/probe/endpoint/nat_internal_test.go @@ -30,7 +30,7 @@ func TestNat(t *testing.T) { // the setup is this: // // container2 (10.0.47.2:222222), host2 (2.3.4.5:22223) -> - // host1 (1.2.3.4:80), container1 (10.0.47.2:80) + // host1 (1.2.3.4:80), container1 (10.0.47.1:80) // from the PoV of host1 { diff --git a/probe/endpoint/reporter.go b/probe/endpoint/reporter.go index c1df1eedf..8f2fcf2c0 100644 --- a/probe/endpoint/reporter.go +++ b/probe/endpoint/reporter.go @@ -118,27 +118,20 @@ func (r *Reporter) Report() (report.Report, error) { uint16(f.Original.Layer4.SrcPort), uint16(f.Original.Layer4.DstPort), } - seenTuples[tuple.key()] = tuple - r.addConnection(&rpt, tuple, extraNodeInfo, extraNodeInfo) - // Handle DNAT-ed short-lived connections. // The NAT mapper won't help since it only runs periodically, // missing the short-lived connections. if f.Original.Layer3.DstIP != f.Reply.Layer3.SrcIP { - reply_tuple := fourTuple{ + tuple = fourTuple{ f.Reply.Layer3.DstIP, f.Reply.Layer3.SrcIP, uint16(f.Reply.Layer4.DstPort), uint16(f.Reply.Layer4.SrcPort), } - // FIXME: For DNAT-ed connections the - // f.Original.Layer3.SrcIP -> f.Original.Layer3.DstIP connection above - // results in a unattributed dangling-edge to the Internet node. - // It could be enough to simply not add that connection, but - // it may not be correct in general. - r.addConnection(&rpt, reply_tuple, extraNodeInfo, extraNodeInfo) } + seenTuples[tuple.key()] = tuple + r.addConnection(&rpt, tuple, extraNodeInfo, extraNodeInfo) }) } From 4140d288a8cb5e1fe53a3327dac2be4f834c2262 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 4 May 2016 13:51:51 +0000 Subject: [PATCH 3/6] Don't scope docker IPs in Kubernetes --- probe/docker/container.go | 2 +- prog/probe.go | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/probe/docker/container.go b/probe/docker/container.go index 5b4fd29c1..7a7292f2a 100644 --- a/probe/docker/container.go +++ b/probe/docker/container.go @@ -298,7 +298,7 @@ func (c *container) NetworkMode() (string, bool) { func addScopeToIPs(hostID string, ips []string) []string { ipsWithScopes := []string{} for _, ip := range ips { - ipsWithScopes = append(ipsWithScopes, report.MakeScopedAddressNodeID(hostID, ip)) + ipsWithScopes = append(ipsWithScopes, report.MakeAddressNodeID(hostID, ip)) } return ipsWithScopes } diff --git a/prog/probe.go b/prog/probe.go index 075255f97..0b45ee574 100644 --- a/prog/probe.go +++ b/prog/probe.go @@ -134,8 +134,12 @@ func probeMain(flags probeFlags) { p.AddTagger(probe.NewTopologyTagger(), host.NewTagger(hostID)) if flags.dockerEnabled { - if err := report.AddLocalBridge(flags.dockerBridge); err != nil { - log.Errorf("Docker: problem with bridge %s: %v", flags.dockerBridge, err) + // Don't add the bridge in Kubernetes since container IPs are global and + // shouldn't be scoped + if !flags.kubernetesEnabled { + if err := report.AddLocalBridge(flags.dockerBridge); err != nil { + log.Errorf("Docker: problem with bridge %s: %v", flags.dockerBridge, err) + } } if registry, err := docker.NewRegistry(flags.dockerInterval, clients, true, hostID); err == nil { defer registry.Stop() From 91339aeae03d231ef96f71a95e0739627e0a9b8e Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Wed, 4 May 2016 17:17:42 +0100 Subject: [PATCH 4/6] Detect pause containers correctly --- probe/kubernetes/reporter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/probe/kubernetes/reporter.go b/probe/kubernetes/reporter.go index fcf7ea9fa..948e6214d 100644 --- a/probe/kubernetes/reporter.go +++ b/probe/kubernetes/reporter.go @@ -92,7 +92,7 @@ func (r *Reporter) podEvent(e Event, pod Pod) { } func isPauseContainer(n report.Node, rpt report.Report) bool { - containerImageIDs, ok := n.Sets.Lookup(report.ContainerImage) + containerImageIDs, ok := n.Parents.Lookup(report.ContainerImage) if !ok { return false } From d1ea4aa6f88ba0467bab460b2aa1d37fa029c86a Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 4 May 2016 16:36:37 +0000 Subject: [PATCH 5/6] Fix tests --- probe/docker/container_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/probe/docker/container_test.go b/probe/docker/container_test.go index ca462204b..aefe039fe 100644 --- a/probe/docker/container_test.go +++ b/probe/docker/container_test.go @@ -113,7 +113,7 @@ func TestContainer(t *testing.T) { want := report.EmptySets. Add("docker_container_ports", report.MakeStringSet("1.2.3.4:80->80/tcp", "81/tcp")). Add("docker_container_ips", report.MakeStringSet("1.2.3.4")). - Add("docker_container_ips_with_scopes", report.MakeStringSet("scope;1.2.3.4")) + Add("docker_container_ips_with_scopes", report.MakeStringSet(";1.2.3.4")) test.Poll(t, 100*time.Millisecond, want, func() interface{} { return c.NetworkInfo([]net.IP{}) From 5b2e0a6011463ce69857d9759a281828214ab95e Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 5 May 2016 14:51:23 +0000 Subject: [PATCH 6/6] Add short-lived connection test --- render/short_lived_connections_test.go | 86 +++++++++++++++++++++++--- 1 file changed, 76 insertions(+), 10 deletions(-) diff --git a/render/short_lived_connections_test.go b/render/short_lived_connections_test.go index cf2b133c5..0c147c999 100644 --- a/render/short_lived_connections_test.go +++ b/render/short_lived_connections_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" + "github.com/weaveworks/scope/common/mtime" + "github.com/weaveworks/scope/probe/docker" "github.com/weaveworks/scope/probe/endpoint" "github.com/weaveworks/scope/probe/host" @@ -23,10 +25,27 @@ var ( serverPort = "80" serverEndpointNodeID = report.MakeEndpointNodeID(serverHostID, serverIP, serverPort) - containerID = "a1b2c3d4e5" - containerIP = "192.168.0.1" - containerName = "foo" - containerNodeID = report.MakeContainerNodeID(containerID) + container1ID = "11b2c3d4e5" + container1IP = "192.168.0.1" + container1Name = "foo" + container1NodeID = report.MakeContainerNodeID(container1ID) + + container1Port = "16782" + container1EndpointNodeID = report.MakeEndpointNodeID(serverHostID, container1IP, container1Port) + + duplicatedIP = "192.168.0.2" + duplicatedPort = "80" + duplicatedEndpointNodeID = report.MakeEndpointNodeID(serverHostID, duplicatedIP, duplicatedPort) + + container2ID = "21b2c3d4e5" + container2IP = duplicatedIP + container2Name = "bar" + container2NodeID = report.MakeContainerNodeID(container2ID) + + pauseContainerID = "31b2c3d4e5" + pauseContainerIP = duplicatedIP + pauseContainerName = "POD" + pauseContainerNodeID = report.MakeContainerNodeID(pauseContainerID) rpt = report.Report{ Endpoint: report.Topology{ @@ -44,19 +63,52 @@ var ( endpoint.Conntracked: "true", }). WithTopology(report.Endpoint), + + container1EndpointNodeID: report.MakeNodeWith(container1EndpointNodeID, map[string]string{ + endpoint.Addr: container1IP, + endpoint.Port: container1Port, + endpoint.Conntracked: "true", + }). + WithAdjacent(duplicatedEndpointNodeID).WithTopology(report.Endpoint), + + duplicatedEndpointNodeID: report.MakeNodeWith(duplicatedEndpointNodeID, map[string]string{ + endpoint.Addr: duplicatedIP, + endpoint.Port: duplicatedPort, + endpoint.Conntracked: "true", + }). + WithTopology(report.Endpoint), }, }, Container: report.Topology{ Nodes: report.Nodes{ - containerNodeID: report.MakeNodeWith(containerNodeID, map[string]string{ - docker.ContainerID: containerID, - docker.ContainerName: containerName, + container1NodeID: report.MakeNodeWith(container1NodeID, map[string]string{ + docker.ContainerID: container1ID, + docker.ContainerName: container1Name, report.HostNodeID: serverHostNodeID, }). WithSets(report.EmptySets. - Add(docker.ContainerIPs, report.MakeStringSet(containerIP)). + Add(docker.ContainerIPs, report.MakeStringSet(container1IP)). + Add(docker.ContainerIPsWithScopes, report.MakeStringSet(report.MakeAddressNodeID("", container1IP))). Add(docker.ContainerPorts, report.MakeStringSet(fmt.Sprintf("%s:%s->%s/tcp", serverIP, serverPort, serverPort))), ).WithTopology(report.Container), + container2NodeID: report.MakeNodeWith(container2NodeID, map[string]string{ + docker.ContainerID: container2ID, + docker.ContainerName: container2Name, + report.HostNodeID: serverHostNodeID, + }). + WithSets(report.EmptySets. + Add(docker.ContainerIPs, report.MakeStringSet(container2IP)). + Add(docker.ContainerIPsWithScopes, report.MakeStringSet(report.MakeAddressNodeID("", container2IP))), + ).WithTopology(report.Container), + pauseContainerNodeID: report.MakeNodeWith(pauseContainerNodeID, map[string]string{ + docker.ContainerID: pauseContainerID, + docker.ContainerName: pauseContainerName, + report.HostNodeID: serverHostNodeID, + }). + WithSets(report.EmptySets. + Add(docker.ContainerIPs, report.MakeStringSet(pauseContainerIP)). + Add(docker.ContainerIPsWithScopes, report.MakeStringSet(report.MakeAddressNodeID("", pauseContainerIP))), + ).WithTopology(report.Container).WithLatest(report.DoesNotMakeConnections, mtime.Now(), ""), }, }, Host: report.Topology{ @@ -81,7 +133,21 @@ func TestShortLivedInternetNodeConnections(t *testing.T) { t.Fatal("Expected output to have an incoming internet node") } - if !internet.Adjacency.Contains(report.MakeContainerNodeID(containerID)) { - t.Errorf("Expected internet node to have adjacency to %s, but only had %v", report.MakeContainerNodeID(containerID), internet.Adjacency) + if !internet.Adjacency.Contains(container1NodeID) { + t.Errorf("Expected internet node to have adjacency to %s, but only had %v", container1NodeID, internet.Adjacency) } } + +func TestPauseContainerDiscarded(t *testing.T) { + have := Prune(render.ContainerWithImageNameRenderer.Render(rpt, render.FilterNoop)) + // There should be a single connection between from container1 and the destination should be container2 + container1, ok := have[container1NodeID] + if !ok { + t.Fatal("Expected output to have container1") + } + + if !container1.Adjacency.Contains(container2NodeID) { + t.Errorf("Expected container1 to have adjacency to %s, but only had %v", container2NodeID, container1.Adjacency) + } + +}