Skip to content

Commit

Permalink
server: node connections must not be forwarded
Browse files Browse the repository at this point in the history
This fixes a bug where a forwarded node update request may be assumed
to be the actual direct client connection if the server just lost
leadership.

When a nomad non-leader server receives a Node.UpdateStatus request, it
forwards the RPC request to the leader, and holds on the request
Yamux connection in a cache to allow for server<->client forwarding.

When the leader handles the request, it must differentiate between a
forwarded connection vs the actual connection.  This is done in
https://github.com/hashicorp/nomad/blob/v0.10.4/nomad/node_endpoint.go#L412

Now, consider if the non-leader server forwards to the connection to a
recently deposed nomad leader, which in turn forwards the RPC request to
the new leader.

Without this change, the deposed leader will mistake the forwarded
connection for the actual client connection and cache it mapped to the
client ID.  If the server attempts to connect to that client, it will
attempt to start a connection/session to the other server instead and
the call will hang forever.

This change ensures that we only add node connection mapping if the
request is not a forwarded request, regardless of circumstances.
  • Loading branch information
Mahmood Ali committed Mar 17, 2020
1 parent 0ecda99 commit 429f531
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions nomad/node_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func (n *Node) Register(args *structs.NodeRegisterRequest, reply *structs.NodeUp
// We have a valid node connection since there is no error from the
// forwarded server, so add the mapping to cache the
// connection and allow the server to send RPCs to the client.
if err == nil && n.ctx != nil && n.ctx.NodeID == "" {
if err == nil && n.ctx != nil && n.ctx.NodeID == "" && !args.IsForwarded() {
n.ctx.NodeID = args.Node.ID
n.srv.addNodeConn(n.ctx)
}
Expand Down Expand Up @@ -374,7 +374,7 @@ func (n *Node) UpdateStatus(args *structs.NodeUpdateStatusRequest, reply *struct
// We have a valid node connection since there is no error from the
// forwarded server, so add the mapping to cache the
// connection and allow the server to send RPCs to the client.
if err == nil && n.ctx != nil && n.ctx.NodeID == "" {
if err == nil && n.ctx != nil && n.ctx.NodeID == "" && !args.IsForwarded() {
n.ctx.NodeID = args.NodeID
n.srv.addNodeConn(n.ctx)
}
Expand Down Expand Up @@ -925,7 +925,7 @@ func (n *Node) GetClientAllocs(args *structs.NodeSpecificRequest,
// We have a valid node connection since there is no error from the
// forwarded server, so add the mapping to cache the
// connection and allow the server to send RPCs to the client.
if err == nil && n.ctx != nil && n.ctx.NodeID == "" {
if err == nil && n.ctx != nil && n.ctx.NodeID == "" && !args.IsForwarded() {
n.ctx.NodeID = args.NodeID
n.srv.addNodeConn(n.ctx)
}
Expand Down

0 comments on commit 429f531

Please sign in to comment.