Alternator: "/localnodes" request should list public - not internal - IP addresses

[nyh]

In some Scylla setups, nodes have two network interfaces - an internal network for inter-node communication, and an external network for communicating with clients. Accordingly each node can have two different IP addresses - an internal one and an external one.

@hopugop discovered that Alternator's /localnodes request currently returns a list of the internal IP addresses, but this is wrong - it should return a list of external IP addresses.

[nyh]

We have a configuration option alternator_address which can be used to control which IP address Alternator listens on (see alternator/controller.cc, controller::start_server()), this doesn't necessarily needs to be (but it usually is) the same address which CQL listens on - the awkwardly-named rpc_address option.

For /localnodes, we need to list all other node's alternator_address setting, and I don't know if or how we have access to that. Do we have access to other nodes' CQL listening addresses (rpc_address)?

[nyh]

Looking at https://github.com/scylladb/scylladb/blob/master/docs/kb/yaml-address.rst and https://github.com/scylladb/scylladb/blob/master/docs/operating-scylla/admin.rst I realized now that this area of Scylla is even more complex than I thought. As shown in the above links (and other places including docs/dev/protocols.md), CQL has a whole list of configuration options, most importantly the special option

broadcast_rpc_address: Address that is broadcasted to tell the clients to connect to. Related to rpc_address.

and we gossip this broadcast_rpc_address in service/storage_service.cc as gms::application_state::RPC_ADDRESS, so we can find another nodes' broadcast_rpc_address. We have a shortcut method gossiper.get_rpc_address(endpoint) to use this gossiped address. It's not used a lot, but it's used.

We can probably use that address instead of gossiping also an Alternator address, but we'll need to document that we did this (and I hope that in @hopugop's use case, they actually set broadcast_rpc_address).
To do this we can in alternator/server.cc, local_nodelist_handler::do_handle(), use _gossiper.get_application_state_value(ip, gms::application_state::RPC_ADDRESS) or even better the shortcut, _gossiper.get_rpc_address(ip) instead of ip.to_sstring(). That would be a trivial patch.

[mykaul]

Don't we have all nodes and their rpc_address stored in some system table that the CQL clients fetch? system.peers perhaps?

[nyh]

@hopugop I propose the following patch. The only question is whether in your experience (and your setup), we can assume that alternator_address and broadcast_rpc_address are configured to the same thing - the external IP addresss of the node.

diff --git a/alternator/server.cc b/alternator/server.cc
--- a/alternator/server.cc
+++ b/alternator/server.cc
         std::unordered_set<gms::inet_address> local_dc_nodes = topology.get_datacenter_endpoints().at(local_dc);
         for (auto& ip : local_dc_nodes) {
             if (_gossiper.is_alive(ip)) {
-                rjson::push_back(results, rjson::from_string(ip.to_sstring()));
+                // Use the gossiped broadcast_rpc_address if available instead
+                // of the internal IP address "ip". See discussion in #18711.
+                // Note that currently we don't gossip the alternator_address
+                // configuration, so for now we assume it's not different from
+                // broadcast_rpc_address.
+                rjson::push_back(results, rjson::from_string(_gossiper.get_rpc_address(ip)));
             }
         }
         rep->set_status(reply::status_type::ok);

To be a complete patch, I'll also need to add a (see test/alternator/test_scylla.py for an example test of /localnodes, and test/topology_experimental_raft/test_mv_tablets.py for an example of a multi-node Alternator test). I'll also need to add documentation for "/localnodes" (it seems it's not anywhere in docs/alternator?) and explain there the IP address issue and how to set it.

[nyh]

Don't we have all nodes and their rpc_address stored in some system table that the CQL clients fetch? system.peers perhaps?

Yes, we do, but this is a CQL table so I thought it would be much less efficient than the gossiper's hash table lookup. Although "/localnodes" isn't a very high-performance request (it should be run once every few seconds, not thousands of times per second).

By the way, to be honest, I don't know what the Raft-based topology will do or already did to gossiper::get_rpc_address(ip). I was hoping that even if we end up dropping the gossiper, we'll at least continue to implement this and similar functions (I also use gossiper::is_alive() in the same code) so that code will continue to work - but I'm not really sure what is the plan.

[nyh]

@hopugop I propose the following patch. The only question is whether in your experience (and your setup), we can assume that alternator_address and broadcast_rpc_address are configured to the same thing - the external IP addresss of the node.

@hopugop responded in a different thread that indeed in Scylla Cloud (for example), publicly-accessible clusters have listen_address and broadcast_address set to an internal IP address, rpc_address is just set to 0.0.0.0 (so CQL listens on both interfaces), and broadcast_rpc_address is set to an external IP address. So it makes sense that alternator_address would be set to 0.0.0.0 too (so Alternator too would listen on both interfaces) but what /localnodes should report should be the broadcast_rpc_address.

We can still imagine a theoretical situation where there are two different external networks and broadcast_rpc_address points CQL clients to one, while Alternator clients should be pointed to a different one - but I think this is unlikely and we can ignore this possibility for now - but we do need to document how broadcast_rpc_address affects Alternator too.

[hopugop]

Thanks @nyh. PR #18828 fixes the bug. I think it is not a requirement to backport it - it could be a feature on new releases.