Dynamic RPC Server and Node connection map #3721
Conversation
| n.ctx.NodeID = args.NodeID | ||
| n.srv.addNodeConn(n.ctx) | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a reason why only a few endpoints attempt to add a valid connection like this? for e.g why not do the same thing in GetNode?
There was a problem hiding this comment.
I am only adding it to RPCs that are only ever called by a Nomad node. The mapping should only map connections to clients. So that is why I only added to a few.
nomad/rpc.go
Outdated
|
|
||
| // Parse the region and role from the TLS certificate | ||
| state := tlsConn.ConnectionState() | ||
| parts := strings.SplitN(state.ServerName, ".", 3) |
There was a problem hiding this comment.
Is this name guarenteed to be in this format always?
There was a problem hiding this comment.
It should be in the case of verifying hostnames but it is a bit brittle. Going to rework
There was a problem hiding this comment.
See newest commit. I store the whole verified chain so we can work with the certificates where needed.
nomad/rpc.go
Outdated
| state := tlsConn.ConnectionState() | ||
| parts := strings.SplitN(state.ServerName, ".", 3) | ||
| if len(parts) != 3 || (parts[0] != "server" && parts[0] != "client") || parts[2] != "nomad" { | ||
| s.logger.Printf("[WARN] nomad.rpc: invalid server name %q on verified TLS connection", state.ServerName) |
There was a problem hiding this comment.
This validation is hard to follow, why do we care about parts[2] == "nomad" if we only ever read parts [0] and [1]
dadgar
force-pushed
the
f-server-rpc-dynamic
branch
from
146144a to
a495c83
Compare
|
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 introduces a dynamic RPC server per connection and adds a mapping between node IDs and connections.