Merge pull request #339 from weaveworks/fix-render-map-bug

Fix a subtle bug in render mapping

render/mapping.go

@@ -46,33 +46,47 @@ type PseudoFunc func(srcNodeID string, srcNode RenderableNode, dstNodeID string,
 // shall be omitted from the rendered topology.
 type MapFunc func(RenderableNode) (RenderableNode, bool)
 
-// MapEndpointIdentity maps a endpoint topology node to endpoint RenderableNode
-// node. As it is only ever run on endpoint topology nodes, we can safely
-// assume the presence of certain keys.
+// MapEndpointIdentity maps an endpoint topology node to an endpoint
+// renderable node. As it is only ever run on endpoint topology nodes, we
+// expect that certain keys are present.
 func MapEndpointIdentity(m report.NodeMetadata) (RenderableNode, bool) {
+	addr, ok := m.Metadata[endpoint.Addr]
+	if !ok {
+		return RenderableNode{}, false
+	}
+
+	port, ok := m.Metadata[endpoint.Port]
+	if !ok {
+		return RenderableNode{}, false
+	}
+
 	var (
-		id      = MakeEndpointID(report.ExtractHostID(m), m.Metadata[endpoint.Addr], m.Metadata[endpoint.Port])
-		major   = fmt.Sprintf("%s:%s", m.Metadata[endpoint.Addr], m.Metadata[endpoint.Port])
-		pid, ok = m.Metadata[process.PID]
-		minor   = report.ExtractHostID(m)
-		rank    = major
+		id    = MakeEndpointID(report.ExtractHostID(m), addr, port)
+		major = fmt.Sprintf("%s:%s", addr, port)
+		minor = report.ExtractHostID(m)
+		rank  = major
 	)
 
-	if ok {
-		minor = fmt.Sprintf("%s (%s)", report.ExtractHostID(m), pid)
+	if pid, ok := m.Metadata[process.PID]; ok {
+		minor = fmt.Sprintf("%s (%s)", minor, pid)
 	}
 
 	return NewRenderableNode(id, major, minor, rank, m), true
 }
 
-// MapProcessIdentity maps a process topology node to process RenderableNode node.
-// As it is only ever run on process topology nodes, we can safely assume the
-// presence of certain keys.
+// MapProcessIdentity maps a process topology node to a process renderable
+// node. As it is only ever run on process topology nodes, we expect that
+// certain keys are present.
 func MapProcessIdentity(m report.NodeMetadata) (RenderableNode, bool) {
+	pid, ok := m.Metadata[process.PID]
+	if !ok {
+		return RenderableNode{}, false
+	}
+
 	var (
-		id    = MakeProcessID(report.ExtractHostID(m), m.Metadata[process.PID])
+		id    = MakeProcessID(report.ExtractHostID(m), pid)
 		major = m.Metadata["comm"]
-		minor = fmt.Sprintf("%s (%s)", report.ExtractHostID(m), m.Metadata[process.PID])
+		minor = fmt.Sprintf("%s (%s)", report.ExtractHostID(m), pid)
 		rank  = m.Metadata["comm"]
 	)
 
@@ -80,11 +94,15 @@ func MapProcessIdentity(m report.NodeMetadata) (RenderableNode, bool) {
 }
 
 // MapContainerIdentity maps a container topology node to a container
-// RenderableNode node. As it is only ever run on container topology
-// nodes, we can safely assume the presences of certain keys.
+// renderable node. As it is only ever run on container topology nodes, we
+// expect that certain keys are present.
 func MapContainerIdentity(m report.NodeMetadata) (RenderableNode, bool) {
+	id, ok := m.Metadata[docker.ContainerID]
+	if !ok {
+		return RenderableNode{}, false
+	}
+
 	var (
-		id    = m.Metadata[docker.ContainerID]
 		major = m.Metadata[docker.ContainerName]
 		minor = report.ExtractHostID(m)
 		rank  = m.Metadata[docker.ImageID]
@@ -94,11 +112,15 @@ func MapContainerIdentity(m report.NodeMetadata) (RenderableNode, bool) {
 }
 
 // MapContainerImageIdentity maps a container image topology node to container
-// image RenderableNode node. As it is only ever run on container image
-// topology nodes, we can safely assume the presences of certain keys.
+// image renderable node. As it is only ever run on container image topology
+// nodes, we expect that certain keys are present.
 func MapContainerImageIdentity(m report.NodeMetadata) (RenderableNode, bool) {
+	id, ok := m.Metadata[docker.ImageID]
+	if !ok {
+		return RenderableNode{}, false
+	}
+
 	var (
-		id    = m.Metadata[docker.ImageID]
 		major = m.Metadata[docker.ImageName]
 		rank  = m.Metadata[docker.ImageID]
 	)
@@ -106,13 +128,18 @@ func MapContainerImageIdentity(m report.NodeMetadata) (RenderableNode, bool) {
 	return NewRenderableNode(id, major, "", rank, m), true
 }
 
-// MapAddressIdentity maps a address topology node to address RenderableNode
-// node. As it is only ever run on address topology nodes, we can safely
-// assume the presence of certain keys.
+// MapAddressIdentity maps an address topology node to an address renderable
+// node. As it is only ever run on address topology nodes, we expect that
+// certain keys are present.
 func MapAddressIdentity(m report.NodeMetadata) (RenderableNode, bool) {
+	addr, ok := m.Metadata[endpoint.Addr]
+	if !ok {
+		return RenderableNode{}, false
+	}
+
 	var (
-		id    = MakeAddressID(report.ExtractHostID(m), m.Metadata[endpoint.Addr])
-		major = m.Metadata[endpoint.Addr]
+		id    = MakeAddressID(report.ExtractHostID(m), addr)
+		major = addr
 		minor = report.ExtractHostID(m)
 		rank  = major
 	)
@@ -120,9 +147,9 @@ func MapAddressIdentity(m report.NodeMetadata) (RenderableNode, bool) {
 	return NewRenderableNode(id, major, minor, rank, m), true
 }
 
-// MapHostIdentity maps a host topology node to host RenderableNode
-// node. As it is only ever run on host topology nodes, we can safely
-// assume the presence of certain keys.
+// MapHostIdentity maps a host topology node to a host renderable node. As it
+// is only ever run on host topology nodes, we expect that certain keys are
+// present.
 func MapHostIdentity(m report.NodeMetadata) (RenderableNode, bool) {
 	var (
 		id                 = MakeHostID(report.ExtractHostID(m))

render/mapping_test.go

@@ -0,0 +1,78 @@
+package render_test
+
+import (
+	"testing"
+
+	"github.com/weaveworks/scope/probe/docker"
+	"github.com/weaveworks/scope/probe/endpoint"
+	"github.com/weaveworks/scope/probe/process"
+	"github.com/weaveworks/scope/render"
+	"github.com/weaveworks/scope/report"
+)
+
+func TestMapEndpointIdentity(t *testing.T) {
+	for _, input := range []testcase{
+		{report.MakeNodeMetadata(), false},
+		{report.MakeNodeMetadataWith(map[string]string{endpoint.Addr: "1.2.3.4"}), false},
+		{report.MakeNodeMetadataWith(map[string]string{endpoint.Port: "1234"}), false},
+		{report.MakeNodeMetadataWith(map[string]string{endpoint.Addr: "1.2.3.4", endpoint.Port: "1234"}), true},
+		{report.MakeNodeMetadataWith(map[string]string{report.HostNodeID: report.MakeHostNodeID("foo"), endpoint.Addr: "10.0.0.1", endpoint.Port: "20001"}), true},
+	} {
+		testMap(t, render.MapEndpointIdentity, input)
+	}
+}
+
+func TestMapProcessIdentity(t *testing.T) {
+	for _, input := range []testcase{
+		{report.MakeNodeMetadata(), false},
+		{report.MakeNodeMetadataWith(map[string]string{process.PID: "201"}), true},
+	} {
+		testMap(t, render.MapProcessIdentity, input)
+	}
+}
+
+func TestMapContainerIdentity(t *testing.T) {
+	for _, input := range []testcase{
+		{report.MakeNodeMetadata(), false},
+		{report.MakeNodeMetadataWith(map[string]string{docker.ContainerID: "a1b2c3"}), true},
+	} {
+		testMap(t, render.MapContainerIdentity, input)
+	}
+}
+
+func TestMapContainerImageIdentity(t *testing.T) {
+	for _, input := range []testcase{
+		{report.MakeNodeMetadata(), false},
+		{report.MakeNodeMetadataWith(map[string]string{docker.ImageID: "a1b2c3"}), true},
+	} {
+		testMap(t, render.MapContainerImageIdentity, input)
+	}
+}
+
+func TestMapAddressIdentity(t *testing.T) {
+	for _, input := range []testcase{
+		{report.MakeNodeMetadata(), false},
+		{report.MakeNodeMetadataWith(map[string]string{endpoint.Addr: "192.168.1.1"}), true},
+	} {
+		testMap(t, render.MapAddressIdentity, input)
+	}
+}
+
+func TestMapHostIdentity(t *testing.T) {
+	for _, input := range []testcase{
+		{report.MakeNodeMetadata(), true}, // TODO it's questionable if this is actually correct
+	} {
+		testMap(t, render.MapHostIdentity, input)
+	}
+}
+
+type testcase struct {
+	md report.NodeMetadata
+	ok bool
+}
+
+func testMap(t *testing.T, f render.LeafMapFunc, input testcase) {
+	if _, have := f(input.md); input.ok != have {
+		t.Errorf("%v: want %v, have %v", input.md, input.ok, have)
+	}
+}

report/topology.go

@@ -89,7 +89,6 @@ func (t Topology) Validate() error {
 		}
 		if _, ok := t.NodeMetadatas[srcNodeID]; !ok {
 			errs = append(errs, fmt.Sprintf("node metadata missing for source node ID %q (from edge %q)", srcNodeID, edgeID))
-			continue
 		}
 		dstNodeIDs, ok := t.Adjacency[MakeAdjacencyID(srcNodeID)]
 		if !ok {
@@ -98,7 +97,6 @@ func (t Topology) Validate() error {
 		}
 		if !dstNodeIDs.Contains(dstNodeID) {
 			errs = append(errs, fmt.Sprintf("adjacency destination missing for destination node ID %q (from edge %q)", dstNodeID, edgeID))
-			continue
 		}
 	}
 
@@ -111,20 +109,22 @@ func (t Topology) Validate() error {
 		}
 		if _, ok := t.NodeMetadatas[nodeID]; !ok {
 			errs = append(errs, fmt.Sprintf("node metadata missing for source node %q (from adjacency %q)", nodeID, adjacencyID))
-			continue
 		}
 	}
 
-	// Check all node metadata keys are parse-able (i.e. contain a scope)
+	// Check all node metadatas are valid, and the keys are parseable, i.e.
+	// contain a scope.
 	for nodeID := range t.NodeMetadatas {
+		if t.NodeMetadatas[nodeID].Metadata == nil {
+			errs = append(errs, fmt.Sprintf("node ID %q has nil metadata", nodeID))
+		}
 		if _, _, ok := ParseNodeID(nodeID); !ok {
 			errs = append(errs, fmt.Sprintf("invalid node ID %q", nodeID))
-			continue
 		}
 	}
 
 	if len(errs) > 0 {
-		return fmt.Errorf(strings.Join(errs, "; "))
+		return fmt.Errorf("%d error(s): %s", len(errs), strings.Join(errs, "; "))
 	}
 
 	return nil