message: messaging_service: topology independent connection settings for GOSSIP verbs #11998
Conversation
…for GOSSIP verbs The gossip verbs are used to learn about topology of other nodes. If inter-dc/rack encryption is enabled, the knowledge of topology is necessary to decide whether it's safe to send unencrypted messages to nodes (i.e., whether the destination lies in the same dc/rack). The logic in `messaging_service::get_rpc_client`, which decided whether a connection must be encrypted, was this (given that encryption is enabled): if the topology of the peer is known, and the peer is in the same dc/rack, don't encrypt. Otherwise encrypt. However, it may happen that node A knows node B's topology, but B doesn't know A's topology. A deduces that B is in the same DC and rack and tries sending B an unencrypted message. As the code currently stands, this would cause B to call `on_internal_error`. This is what I encountered when attempting to fix scylladb#11780. To guarantee that it's always possible to deliver gossiper verbs (even if one or both sides don't know each other's topology), and to simplify reasoning about the system in general, choose connection settings that are independent of the topology - for the connection used by gossiper verbs (other connections are still topology-dependent and use complex logic to handle the situation of unknown-and-later-known topology). This connection only contains 'rare' and 'cheap' verbs, so it's not a performance problem to always encrypt it (given that encryption is configured). And this is what already was happening in the past; it was at some point removed during topology knowledge management refactors. We just bring this logic back. Fixes scylladb#11992. Inspired by xemul/scylla@45d48f3.
kbr-
changed the title
| }(); | ||
|
|
||
| auto must_tcp_nodelay = [&] { | ||
| if (idx == 0) { |
There was a problem hiding this comment.
This check has nothing to do with "topology independence day ". It's just to make gossiper (see e.g. #11490) work smoother
There was a problem hiding this comment.
It makes gossiper works smoother and makes the setting independent of topology for idx=0.
There was a problem hiding this comment.
I mention tcp_nodelay at the TOPOLOGY_INDEPENDENT_IDX comment too.
| @@ -753,22 +776,21 @@ shared_ptr<messaging_service::rpc_protocol_client_wrapper> messaging_service::ge | |||
| return false; | |||
| } | |||
|
|
|||
| if (_cfg.compress == compress_what::all || !has_topology()) { | |||
| // See comment above `TOPOLOGY_INDEPENDENT_IDX`. | |||
| if (_cfg.compress == compress_what::all || idx == TOPOLOGY_INDEPENDENT_IDX) { | |||
There was a problem hiding this comment.
Why does compression has anything to do with this? If A knows B's topo, but not the vice-versa, would B's compressed message sent to A make any harm?
There was a problem hiding this comment.
It's done to simplify, as explained at TOPOLOGY_INDEPENDENT_IDX comment:
// And since we fixed a topology-independent setting for encryption and tcp_nodelay,
// to keep things simple, we also fix a setting for compression. This allows this RPC client
// to be established without checking the topology (which may not be known anyway
// when we first start gossiping).
This allows us to avoid the RPC dropping for IDX=0 altogether.
|
CI state |
|
It appears that the only controversial thing in this patch is compression settings. The main goal of the patch was to make encryption topology-independent for idx=0 rpc client. tcp_nodelay already is. So compression is the last thing. So I decided to make it consistent with the other two settings so that all settings, including compression, are topology-independent. Thanks to this we don't need to drop the RPC client later. Advantages:
Disadvantages:
Compression could remain topology-dependent. It will be kind of awkward - dropping the RPC client just so it can be recreated with different compression settings, which doesn't really matter for this group of verbs. But we can do it. I don't have a strong opinion on this. @xemul how do we proceed? |
|
I'd go with static configuration for index 0 |
|
@xemul do you think the patch is ready for merging then? |
|
Yes, I was going to merge it as a part of #11942 |
The gossip verbs are used to learn about topology of other nodes. If inter-dc/rack encryption is enabled, the knowledge of topology is necessary to decide whether it's safe to send unencrypted messages to nodes (i.e., whether the destination lies in the same dc/rack).
The logic in
messaging_service::get_rpc_client, which decided whether a connection must be encrypted, was this (given that encryption is enabled): if the topology of the peer is known, and the peer is in the same dc/rack, don't encrypt. Otherwise encrypt.However, it may happen that node A knows node B's topology, but B doesn't know A's topology. A deduces that B is in the same DC and rack and tries sending B an unencrypted message. As the code currently stands, this would cause B to call
on_internal_error. This is what I encountered when attempting to fix #11780.To guarantee that it's always possible to deliver gossiper verbs (even if one or both sides don't know each other's topology), and to simplify reasoning about the system in general, choose connection settings that are independent of the topology - for the connection used by gossiper verbs (other connections are still topology-dependent and use complex logic to handle the situation of unknown-and-later-known topology).
This connection only contains 'rare' and 'cheap' verbs, so it's not a performance problem to always encrypt it (given that encryption is configured). And this is what already was happening in the past; it was at some point removed during topology knowledge management refactors. We just bring this logic back.
Fixes #11992.
Inspired by xemul/scylla@45d48f3.