Fix IPv6 support in `Xandra.Cluster` #328

jacquelineIO · 2023-08-31T17:28:40Z

Fix node validation errors
use rpc_address when available

Xandra will now use tuples for IP addresses instead of strings, which are valid Erlang types. And the tuples are able to run through node validations.

History of why we started working on these changes:

Started this work on 0.16 of Xandra with this PR. Even with these changes we still encountered problems with deploying and Karl opened up an issue with Xandra.

New changes were released for Xandra and reapplying the old PR changes to v0.17 of Xandra.

We are using this lib and the host structs are returning ipv6 addresses for our clusters. The validate_node function is failing on ipv6 addresses

"The pool supervisor will run the validations again on the ipv6 values returned in https://github.com/lexhide/xandra/blob/v0.16.0/lib/xandra/cluster.ex#L793 and in the logs we see that our hosts are returned with both ipv4 and ipv6 addresses. The ipv6 addresses are being used and fail to validate." - @britto

invalid value for list element at position 0: invalid node: \"xxxx:xxxx:xxxx:xxxx:0:xxx:xxxx:xxxx:xxxxx\"", value: ["xxxx:xxxx:xxxx:xxxx:0:xxx:xxxx:xxxx:xxxxx"]}, [{NimbleOptions, :validate!, 2, [file: 'lib/nimble_options.ex', line: 343]}

     (xandra 0.16.0) lib/xandra/cluster.ex:750: Xandra.Cluster.start_pool/2
     (xandra 0.16.0) lib/xandra/cluster.ex:798: anonymous fn/4 in Xandra.Cluster.maybe_start_pools/1
     (elixir 1.12.3) lib/enum.ex:4286: Enumerable.List.reduce/3
     (elixir 1.12.3) lib/enum.ex:2431: Enum.reduce_while/3
     (xandra 0.16.0) lib/xandra/cluster.ex:726: Xandra.Cluster.handle_info/2
     (stdlib 3.17.2) gen_server.erl:695: :gen_server.try_dispatch/4
     (stdlib 3.17.2) gen_server.erl:771: :gen_server.handle_msg/6

These changes are tagged v0.17.x-community.1.

…peer_address configuration

relistan · 2023-09-01T08:45:16Z

We don't just need to validate the IPv6 addresses. As shown here, we need to actually be able to use the RPC addresses because the addresses shown in peer are not routable.

whatyouhide · 2023-09-01T09:51:41Z

lib/xandra/cluster.ex

@@ -345,6 +345,15 @@ defmodule Xandra.Cluster do
      *Available since v0.18.0*.
      """
    ],
+    use_rpc_address_for_peer_address: [


What are the cases when we'd want to use peer instead of rpc_address?

peer was the original implementation. I don't know why you would use peer instead of rpc_address and not much information when I google this, so going to phone a friend. @relistan do you know or think this should just solely use rpc_address and no longer use peer? (I think that is what @whatyouhide is asking).

Yes that's what I’m saying. @jacquelineIO I also recommend doing just what any other C* driver maintained by Datastax is doing. I always reference the Python driver. I don't have much time to look into this until maybe Sunday, so if you want you can just take a look there and use what they're using 😉

Looks like it is using rpc_address with a fallback to peer if rpc_address isn't available

https://github.com/datastax/python-driver/blob/8ba0a5ccd71b66c77ee58994ed9da9ea34ff4cbe/cassandra/cluster.py#L3782

https://github.com/datastax/python-driver/blob/8ba0a5ccd71b66c77ee58994ed9da9ea34ff4cbe/cassandra/metadata.py#L3381

So I can remove this flag and have a preference for using rpc_address, which the code is basically already set up to do

Yes, let's do that so that we don't have to put this decision on users 🙃 Thanks!

Sorry, catching up here. That sounds right to me, then.

@jacquelineIO any news on this and timeline on when I can expect these changes to go in? I'm planning the 0.18.0 release, that's why I ask 🙃

whatyouhide · 2023-09-01T12:36:19Z

lib/xandra/cluster/control_connection.ex

+    queried_peer_to_host(peer_attrs, use_rpc_address)
+  end
+
+  defp queried_peer_to_host(%{"rpc_address" => _} = peer_attrs, true = use_rpc_address) do


When is it that we get rpc_address as a string? Xandra should return :inet.ip_address/0 for IPs, with a 4-element tuple for IPv4 and 8-element tuple for IPv6.

When the peers table is queried, it was using the string.

For example this was one of the logs (when address is ipv6 this was another issue because the validation for ipv6 strings fails in xandra)

[] 21:35:58.910 [info] Xandra: [:connected]: %{address: '10.4.0.12', connection_name: nil, port: 11172, protocol_module: Xandra.Protocol.V4, supported_options: %{"COMPRESSION" => ["snappy", "lz4"], "CQL_VERSION" => ["3.4.5"], "PROTOCOL_VERSIONS" => ["3/v3", "4/v4", "5/v5", "6/v6-beta"]}}

Now the log looks like

[] 16:52:41.083 [info] Xandra Cluster: [:change_event]: %{cluster_name: {:local, FastSummariesConsumer.Store}, cluster_pid: #PID<0.5488.0>, event_type: :host_added, host: %Xandra.Cluster.Host{address: {10, 4, 0, 12},

There is still another place where strings are being used for the original host for the control connection if I'm not mistaken but it broke a lot of tests trying to change that out. That might be good for a different PR to fix.

This is the other location where string is in use https://github.com/Shimmur/xandra/blob/c563d737b99bcdec195a38408db490fec046a5e8/lib/xandra/cluster/pool.ex#L469 and I think might be good to convert this to a tuple as well.

When the peers table is queried, it was using the string.

I’m confused. If you look at the schema for the system.peers table in C* itself, rpc_address is of type inet, so how can the result of the query to that table include rpc_address as a string?

I'm also adding the validate_node options. There was only one previously and it took a string https://github.com/lexhide/xandra/blob/main/lib/xandra/options_validators.ex#L51
if it were not a string, it would have failed...

I don't know about in this particular case, but we were seeing IPv6 addresses getting returned as strings. This was then causing DNS lookups which failed. @jacquelineIO can confirm if this is the reason or not.

@whatyouhide you are right about the query returning the inet format.
It looks like there are some back n forth translations happening, and maybe it shouldn't be.
https://github.com/lexhide/xandra/blob/96303f07bae820ff693d85f2e1585cb343118e00/lib/xandra/cluster/pool.ex#L464-L466 part of the code is formatting the :node as a string.
e.g.

host = %Xandra.Cluster.Host{address: {10, 4, 0, 12}, port: 1234} Xandra.Cluster.Host.format_address(host) "10.4.0.12:1234"

so conn_options has a string and the id is a tuple. https://github.com/lexhide/xandra/blob/96303f07bae820ff693d85f2e1585cb343118e00/lib/xandra/cluster/pool.ex#L469-L471

When validate_node is run on the node address it is fine because it was a string.

So now I'm confused too. I do not completely follow the flow here...

I assumed this was pulling a string and wanted to make sure it was only using a tuple. BUT then also validate node started failing (if my memory is correct) so I had to update that too. So there might be multiple (at least one more) places in the code that a connection is happening, the first one beingstart_pool...

I got the desired result but the original connection is still a string. I this it makes sense to not use format_address for code (maybe only for logging things).

(I also feel like I'm talking in circles)

@jacquelineIO okay yeah this code I have in Xandra is a little bit messy. I'll do another pass and fix this up later on then, so that we can standardize on having a single way of representing addresses.

@whatyouhide that went in already 8340b4d

Ah cool awesome! Will review this soon then

relistan · 2023-09-06T13:04:25Z

@whatyouhide those test failures seem to have occurred with ScyllaDB. I am suspicious about that. Is there a way to get it to re-run these without committing?

whatyouhide · 2023-09-07T10:10:15Z

@relistan yep I can re-run the CI suite any time, if you can't (top-right on GH Actions) because you don't have commit access to the repo then just ping me and I'll do it for ya 🙃

relistan · 2023-09-07T13:48:05Z

I can't trigger a rebuild. But it looks like @jacquelineIO needs to run mix format so that will trigger a build when she pushes.

jacquelineIO · 2023-09-07T14:55:47Z

So these test runs feel sporadic, I was running the formatter and it was down to 1 build failure (not counting the linter) now it's up to 4. And previously I had none before taking out the option to use rpc_address. So I'm not sure what to do here.

whatyouhide · 2023-09-07T15:02:32Z

We have some flaky tests (check the issue tracker), so I would suggest to not worry about tests too much for now, let's get the changes in and when we're ready I can take a look at the failures and merge even if they're there, no worries 🙃

mattias01 · 2023-09-12T11:17:39Z

The change to use rpc_address instead of peer is a fix we need to be able to use a newer Xandra version, which forces the use of auto discovery. I've created a fork where I change just that, but I saw this PR so I will not create a PR for it myself.
Thanks for your work.

whatyouhide · 2023-09-12T20:56:36Z

Thanks @jacquelineIO! 💟

jacquelineIO added 17 commits August 18, 2023 10:05

use rpc_address

f5bb426

debug validate_node

72c2a3d

add option to configure use of rpc_address, node validation for ipv6

66de9b7

do not format to string, leave address as Host

8b7b5ff

put back call to format_address

744f0cf

debugging

d5dd7e3

debug

02ffeb6

missed some spots for passing through use_rpc_address_for_peer_address

eb2eb9c

use new queried_peer_to_host with use_rpc_address

65df217

remove commented out functions

403abda

remove debug logs

0cbddaf

fix tests

bb3c925

formatter

da011a9

formatter

c563d73

additional test to check both true and false for use_rpc_address_for_…

ce0e7f8

…peer_address configuration

formatter

0299f04

formatter

6b1a5ac

whatyouhide reviewed Sep 1, 2023

View reviewed changes

whatyouhide changed the title ~~ipv6 validation and ipv4 usage on v0.17~~ Fix IPv6 support in Xandra.Cluster Sep 2, 2023

always use rpc_address if available

8340b4d

jacquelineIO added 3 commits September 7, 2023 09:15

formatter

e3259e1

formatter

7b5ca9d

formatter

5545567

whatyouhide merged commit e04ff97 into whatyouhide:main Sep 12, 2023
2 of 6 checks passed

britto deleted the jacqueline/ipv6-handling-changes-on-v17 branch December 29, 2023 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IPv6 support in `Xandra.Cluster` #328

Fix IPv6 support in `Xandra.Cluster` #328

jacquelineIO commented Aug 31, 2023 •

edited

Loading

relistan commented Sep 1, 2023 •

edited

Loading

whatyouhide Sep 1, 2023

jacquelineIO Sep 1, 2023

whatyouhide Sep 1, 2023

jacquelineIO Sep 5, 2023

whatyouhide Sep 6, 2023

relistan Sep 6, 2023

whatyouhide Sep 12, 2023

whatyouhide Sep 1, 2023

jacquelineIO Sep 1, 2023 •

edited

Loading

jacquelineIO Sep 1, 2023

whatyouhide Sep 1, 2023

jacquelineIO Sep 1, 2023

relistan Sep 7, 2023 •

edited

Loading

jacquelineIO Sep 7, 2023

whatyouhide Sep 12, 2023

jacquelineIO Sep 12, 2023

whatyouhide Sep 12, 2023

relistan commented Sep 6, 2023

whatyouhide commented Sep 7, 2023

relistan commented Sep 7, 2023

jacquelineIO commented Sep 7, 2023

whatyouhide commented Sep 7, 2023

mattias01 commented Sep 12, 2023

whatyouhide commented Sep 12, 2023

Fix IPv6 support in Xandra.Cluster #328

Fix IPv6 support in Xandra.Cluster #328

Conversation

jacquelineIO commented Aug 31, 2023 • edited Loading

relistan commented Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquelineIO Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relistan Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relistan commented Sep 6, 2023

whatyouhide commented Sep 7, 2023

relistan commented Sep 7, 2023

jacquelineIO commented Sep 7, 2023

whatyouhide commented Sep 7, 2023

mattias01 commented Sep 12, 2023

whatyouhide commented Sep 12, 2023

Fix IPv6 support in `Xandra.Cluster` #328

Fix IPv6 support in `Xandra.Cluster` #328

jacquelineIO commented Aug 31, 2023 •

edited

Loading

relistan commented Sep 1, 2023 •

edited

Loading

jacquelineIO Sep 1, 2023 •

edited

Loading

relistan Sep 7, 2023 •

edited

Loading