Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a subtle bug in render mapping #339

Merged
merged 1 commit into from
Aug 3, 2015
Merged

Fix a subtle bug in render mapping #339

merged 1 commit into from
Aug 3, 2015

Conversation

peterbourgon
Copy link
Contributor

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.

@paulbellamy
Copy link
Contributor

Worth checking if m.Metadata == nil?

Also, worth a unit test for the map fns?

peterbourgon added a commit that referenced this pull request Jul 31, 2015
@peterbourgon
Revert to correct edge construction
e59584a 
Another implicit invariant in the data model is that edges are always of the
form (local -> remote). That is, the source of an edge must always be a node
that originates from within Scope's domain of visibility. This was evident by
the presence of ingress and egress fields in edge/aggregate metadata.

When building the sniffer, I accidentally and incorrectly violated this
invariant, by constructing distinct edges for (local -> remote) and (remote ->
local), and collapsing ingress and egress byte counts to a single scalar. I
experienced a variety of subtle undefined behavior as a result. See #339.

This change reverts to the old, correct methodology. Consequently the sniffer
needs to be able to find out which side of the sniffed packet is local v.
remote, and to do that it needs access to local networks. I moved the
discovery from the probe/host package into probe/main.go.

As part of that work I discovered that package report also maintains its own,
independent "cache" of local networks. Except it contains only the (optional)
Docker bridge network, if it's been populated by the probe, and it's only used
by the report.Make{Endpoint,Address}NodeID constructors to scope local
addresses. Normally, scoping happens during rendering, and only for pseudo
nodes -- see current LeafMap Render localNetworks. This is pretty convoluted
and should be either be made consistent or heavily commented.
peterbourgon added a commit that referenced this pull request Jul 31, 2015
@peterbourgon
Revert to correct edge construction
14fbc26 
Another implicit invariant in the data model is that edges are always of the
form (local -> remote). That is, the source of an edge must always be a node
that originates from within Scope's domain of visibility. This was evident by
the presence of ingress and egress fields in edge/aggregate metadata.

When building the sniffer, I accidentally and incorrectly violated this
invariant, by constructing distinct edges for (local -> remote) and (remote ->
local), and collapsing ingress and egress byte counts to a single scalar. I
experienced a variety of subtle undefined behavior as a result. See #339.

This change reverts to the old, correct methodology. Consequently the sniffer
needs to be able to find out which side of the sniffed packet is local v.
remote, and to do that it needs access to local networks. I moved the
discovery from the probe/host package into probe/main.go.

As part of that work I discovered that package report also maintains its own,
independent "cache" of local networks. Except it contains only the (optional)
Docker bridge network, if it's been populated by the probe, and it's only used
by the report.Make{Endpoint,Address}NodeID constructors to scope local
addresses. Normally, scoping happens during rendering, and only for pseudo
nodes -- see current LeafMap Render localNetworks. This is pretty convoluted
and should be either be made consistent or heavily commented.
peterbourgon added a commit that referenced this pull request Jul 31, 2015
@peterbourgon
Revert to correct edge construction
0a2c0c3 
Another implicit invariant in the data model is that edges are always of the
form (local -> remote). That is, the source of an edge must always be a node
that originates from within Scope's domain of visibility. This was evident by
the presence of ingress and egress fields in edge/aggregate metadata.

When building the sniffer, I accidentally and incorrectly violated this
invariant, by constructing distinct edges for (local -> remote) and (remote ->
local), and collapsing ingress and egress byte counts to a single scalar. I
experienced a variety of subtle undefined behavior as a result. See #339.

This change reverts to the old, correct methodology. Consequently the sniffer
needs to be able to find out which side of the sniffed packet is local v.
remote, and to do that it needs access to local networks. I moved the
discovery from the probe/host package into probe/main.go.

As part of that work I discovered that package report also maintains its own,
independent "cache" of local networks. Except it contains only the (optional)
Docker bridge network, if it's been populated by the probe, and it's only used
by the report.Make{Endpoint,Address}NodeID constructors to scope local
addresses. Normally, scoping happens during rendering, and only for pseudo
nodes -- see current LeafMap Render localNetworks. This is pretty convoluted
and should be either be made consistent or heavily commented.
@peterbourgon
Copy link
Contributor Author

Worth checking if m.Metadata == nil?

A NodeMetadata with a nil Metadata field is always invalid. I've added that constraint to the topology validator.

Also, worth a unit test for the map fns?

👍

@peterbourgon
Fix a subtle bug in render mapping
b3868c5 
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
peterbourgon added a commit that referenced this pull request Aug 3, 2015
@peterbourgon
Merge pull request #339 from weaveworks/fix-render-map-bug
8fdbc44 
Fix a subtle bug in render mapping
@peterbourgon peterbourgon merged commit 8fdbc44 into master Aug 3, 2015
@peterbourgon peterbourgon deleted the fix-render-map-bug branch August 3, 2015 08:53
peterbourgon added a commit that referenced this pull request Aug 3, 2015
@peterbourgon
Revert to correct edge construction
0aadf64 
Another implicit invariant in the data model is that edges are always of the
form (local -> remote). That is, the source of an edge must always be a node
that originates from within Scope's domain of visibility. This was evident by
the presence of ingress and egress fields in edge/aggregate metadata.

When building the sniffer, I accidentally and incorrectly violated this
invariant, by constructing distinct edges for (local -> remote) and (remote ->
local), and collapsing ingress and egress byte counts to a single scalar. I
experienced a variety of subtle undefined behavior as a result. See #339.

This change reverts to the old, correct methodology. Consequently the sniffer
needs to be able to find out which side of the sniffed packet is local v.
remote, and to do that it needs access to local networks. I moved the
discovery from the probe/host package into probe/main.go.

As part of that work I discovered that package report also maintains its own,
independent "cache" of local networks. Except it contains only the (optional)
Docker bridge network, if it's been populated by the probe, and it's only used
by the report.Make{Endpoint,Address}NodeID constructors to scope local
addresses. Normally, scoping happens during rendering, and only for pseudo
nodes -- see current LeafMap Render localNetworks. This is pretty convoluted
and should be either be made consistent or heavily commented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@peterbourgon peterbourgon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peterbourgon @paulbellamy