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.

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

Fix RPC tunneling when running both client/server #4317

Merged
merged 5 commits into from
May 22, 2018
Merged

Fix RPC tunneling when running both client/server #4317

merged 5 commits into from
May 22, 2018

Conversation

dadgar
Copy link
Contributor

@dadgar dadgar commented May 21, 2018

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

conf.RPCHandler = a.server
conf.Servers = append(conf.Servers,
a.config.Addresses.RPC,
a.config.AdvertiseAddrs.RPC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make a unit test for tunneling that runs client/server on the same node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@chelseakomlo chelseakomlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple questions but looks good overall. It would be great to have tests for these edge cases as well (both for the client/server case and when a specific nodeID is not found in a server's node connections).

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worthwhile to add a test explicitly for this error case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_, ok = s1.getNodeConn(nodeID)
require.False(ok)

This just reduces nosy logs.

if a.server != nil {
conf.RPCHandler = a.server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was setting the RPCHandler removed? What replaces this functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if a.server != nil {
conf.RPCHandler = a.server
conf.Servers = append(conf.Servers,
a.config.Addresses.RPC,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't ever be nil as it is materialized well before we get here. See normalizeAddrs in command/command.go. Added safety guards

Copy link
Contributor Author

@dadgar dadgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_, ok = s1.getNodeConn(nodeID)
require.False(ok)

This just reduces nosy logs.

if a.server != nil {
conf.RPCHandler = a.server
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dadgar dadgar merged commit e108f73 into master May 22, 2018
@dadgar dadgar deleted the b-no-path branch May 22, 2018 21:47
@preetapan preetapan mentioned this pull request May 30, 2018
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No stats in alloc-status in nomad 0.8.1 (no path to node)
3 participants