Fix RPC tunneling when running both client/server #4317
Conversation
command/agent/agent.go
Outdated
| conf.RPCHandler = a.server | ||
| conf.Servers = append(conf.Servers, | ||
| a.config.Addresses.RPC, | ||
| a.config.AdvertiseAddrs.RPC) |
There was a problem hiding this comment.
make a unit test for tunneling that runs client/server on the same node?
| @@ -31,6 +31,9 @@ func (s *Server) getNodeConn(nodeID string) (*nodeConnState, bool) { | |||
| s.nodeConnsLock.RLock() | |||
| defer s.nodeConnsLock.RUnlock() | |||
| conns, ok := s.nodeConns[nodeID] | |||
| if !ok { | |||
There was a problem hiding this comment.
Might be worthwhile to add a test explicitly for this error case
There was a problem hiding this comment.
nomad/nomad/client_rpc_test.go
Lines 81 to 82 in 9e761d4
This just reduces nosy logs.
| if a.server != nil { | ||
| conf.RPCHandler = a.server |
There was a problem hiding this comment.
Why was setting the RPCHandler removed? What replaces this functionality?
There was a problem hiding this comment.
If you read my description you will see why this is problematic. By using in memory only RPCs the mechanism to track client connections doesn't work and thus proxied connections break. Therefore we switch to using the normal mechanism of RPCs and just explicitly seed the client's server list with the address the server is bound to.
command/agent/agent.go
Outdated
| if a.server != nil { | ||
| conf.RPCHandler = a.server | ||
| conf.Servers = append(conf.Servers, | ||
| a.config.Addresses.RPC, |
There was a problem hiding this comment.
We don't check whether Addresses and AdvertiseAddrs could ever be nil- are these fields always set elsewhere in the code, or would it be valuable to have a check for nil beforehand?
There was a problem hiding this comment.
Shouldn't ever be nil as it is materialized well before we get here. See normalizeAddrs in command/command.go. Added safety guards
There was a problem hiding this comment.
@chelseakomlo You can see this file for unit test coverage: https://github.com/hashicorp/nomad/blob/9e761d404998380f002dd7282e9eb665e77b8f74/nomad/client_rpc_test.go
| @@ -31,6 +31,9 @@ func (s *Server) getNodeConn(nodeID string) (*nodeConnState, bool) { | |||
| s.nodeConnsLock.RLock() | |||
| defer s.nodeConnsLock.RUnlock() | |||
| conns, ok := s.nodeConns[nodeID] | |||
| if !ok { | |||
There was a problem hiding this comment.
nomad/nomad/client_rpc_test.go
Lines 81 to 82 in 9e761d4
This just reduces nosy logs.
| if a.server != nil { | ||
| conf.RPCHandler = a.server |
There was a problem hiding this comment.
If you read my description you will see why this is problematic. By using in memory only RPCs the mechanism to track client connections doesn't work and thus proxied connections break. Therefore we switch to using the normal mechanism of RPCs and just explicitly seed the client's server list with the address the server is bound to.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR fixes an issue in which clients running in the same process as the
servers (when configured such that server and client are enabled in the
config), would never be registered in the mapping of the node ID to TCP
connection because the client was issuing RPCs directly in-memory.
Fixes #4203