Fix a subtle bug in render mapping

During rendering, RenderableNodes are created by Map funcs, which take only
NodeMetadata. Previously, it was simply assumed that the relevant keys for a
given Map func would be present in the metadata. If that implicit invariant
failed, the returned RenderableNode would be invalid, with e.g. an ID of
"hostid::". That, in turn, would trigger undefined behavior later on in the
rendering workflow.

This bug was detected by creating a partial node metadata for a non-local
endpoint node. That node was detected during the first phase of rendering, and
given an invalid renderable node ID of "myhostname::", which prevented it from
attaching to TheInternet pseudonode. It eventually got removed from the  set
of valid nodes, which meant nodes that were adjacent to it suddenly became
orphans, and got filtered out by the FilterUnconnected step of the rendering
pipeline.

With this change, every map func checks for the presence of mandatory fields,
i.e. the fields that compose the resulting renderable node's ID.

Also,

- Add unit tests for LeafMapFuncs
- Topology Validate checks NodeMetadatas must not have nil Metadata

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