From e81a19ed4df2cebd7744f8515ad60e3271606029 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Wed, 4 Dec 2024 14:57:52 -0500 Subject: [PATCH 1/4] fix: change type of transport_options to support non-keyword options If you want to support IPv6 connections, you need to support non-keyword options that :gen_tcp supports. https://www.erlang.org/docs/19/man/gen_tcp#type-option connect_option() supports inet:address_family() which is inet | inet6 | local. --- lib/xandra.ex | 2 +- lib/xandra/connection.ex | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/xandra.ex b/lib/xandra.ex index a4f0087e..3cbee421 100644 --- a/lib/xandra.ex +++ b/lib/xandra.ex @@ -444,7 +444,7 @@ defmodule Xandra do """ ], transport_options: [ - type: :keyword_list, + type: {:list, {:or, [:keyword, :atom, :any]}}, doc: """ Options to forward to the socket transport. If the `:encryption` option is `true`, then the transport is SSL (see the Erlang `:ssl` module) otherwise it's diff --git a/lib/xandra/connection.ex b/lib/xandra/connection.ex index 86072a37..d0d40cbd 100644 --- a/lib/xandra/connection.ex +++ b/lib/xandra/connection.ex @@ -409,16 +409,21 @@ defmodule Xandra.Connection do nil -> data.original_options end + # Construct the transport options. + {options, other_options} = + options + |> Keyword.get(:transport_options, []) + |> Enum.split_with(fn x -> Keyword.keyword?([x]) end) + # Now, build the state from the options. {address, port} = Keyword.fetch!(options, :node) transport = %Transport{ module: if(options[:encryption], do: :ssl, else: :gen_tcp), options: - options - |> Keyword.get(:transport_options, []) - |> Keyword.put_new(:buffer, @default_transport_buffer_size) - |> Keyword.merge(@forced_transport_options) + (options + |> Keyword.put_new(:buffer, @default_transport_buffer_size) + |> Keyword.merge(@forced_transport_options)) ++ other_options } data = %__MODULE__{ From f5e80520bd29f193dd1d58b859a0315f572fc0b4 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Wed, 4 Dec 2024 15:17:47 -0500 Subject: [PATCH 2/4] fix: add tests for IPv6 transports --- lib/xandra.ex | 2 +- lib/xandra/connection.ex | 4 ++-- test/xandra/transport_test.exs | 12 ++++++++++++ test/xandra_test.exs | 3 +++ 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/xandra.ex b/lib/xandra.ex index 3cbee421..600c8199 100644 --- a/lib/xandra.ex +++ b/lib/xandra.ex @@ -444,7 +444,7 @@ defmodule Xandra do """ ], transport_options: [ - type: {:list, {:or, [:keyword, :atom, :any]}}, + type: {:or, [:keyword_list, {:list, :any}]}, doc: """ Options to forward to the socket transport. If the `:encryption` option is `true`, then the transport is SSL (see the Erlang `:ssl` module) otherwise it's diff --git a/lib/xandra/connection.ex b/lib/xandra/connection.ex index d0d40cbd..a968ac23 100644 --- a/lib/xandra/connection.ex +++ b/lib/xandra/connection.ex @@ -410,7 +410,7 @@ defmodule Xandra.Connection do end # Construct the transport options. - {options, other_options} = + {keyword_options, other_options} = options |> Keyword.get(:transport_options, []) |> Enum.split_with(fn x -> Keyword.keyword?([x]) end) @@ -421,7 +421,7 @@ defmodule Xandra.Connection do transport = %Transport{ module: if(options[:encryption], do: :ssl, else: :gen_tcp), options: - (options + (keyword_options |> Keyword.put_new(:buffer, @default_transport_buffer_size) |> Keyword.merge(@forced_transport_options)) ++ other_options } diff --git a/test/xandra/transport_test.exs b/test/xandra/transport_test.exs index 98278563..798c23c8 100644 --- a/test/xandra/transport_test.exs +++ b/test/xandra/transport_test.exs @@ -17,6 +17,18 @@ defmodule Xandra.TransportTest do assert %Transport{} = transport = Transport.close(transport) assert transport.socket == nil end + + test "returns an IPv6-compatible transport" do + assert {:ok, listen_socket} = :gen_tcp.listen(0, [:inet6]) + assert {:ok, port} = :inet.port(listen_socket) + + transport = %Transport{module: :gen_tcp, options: [:inet6]} + assert {:ok, transport} = Transport.connect(transport, ~c"::1", port, 5000) + assert transport.socket != nil + + assert %Transport{} = transport = Transport.close(transport) + assert transport.socket == nil + end end describe "is_* macros" do diff --git a/test/xandra_test.exs b/test/xandra_test.exs index 854e76c9..dbb251cf 100644 --- a/test/xandra_test.exs +++ b/test/xandra_test.exs @@ -33,6 +33,9 @@ defmodule XandraTest do assert_raise ArgumentError, ~r{the :nodes option can't be an empty list}, fn -> Xandra.start_link(nodes: []) end + + # IPv6 Connection + assert {:ok, _conn} = Xandra.start_link(nodes: ["127.0.0.1"], transport_options: [:inet6]) end test "validates the :authentication option" do From 33056154c1765d6a288bdbe14e9c1c241539becc Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Wed, 4 Dec 2024 16:38:28 -0500 Subject: [PATCH 3/4] fix: update Xandra.Cluster to pass in transport_options correctly and update validate_node to support IPv6 nodes --- lib/xandra/cluster/control_connection.ex | 6 +++- lib/xandra/options_validators.ex | 39 ++++++++++++++++++------ test/xandra/cluster_test.exs | 13 ++++++++ 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/lib/xandra/cluster/control_connection.ex b/lib/xandra/cluster/control_connection.ex index 6ba50319..8bf940c4 100644 --- a/lib/xandra/cluster/control_connection.ex +++ b/lib/xandra/cluster/control_connection.ex @@ -102,9 +102,13 @@ defmodule Xandra.Cluster.ControlConnection do {transport_opts, connection_opts} = Keyword.pop(connection_opts, :transport_options, []) + # Construct the transport options. + {keyword_options, other_options} = + Enum.split_with(transport_opts, fn x -> Keyword.keyword?([x]) end) + transport = %Transport{ module: module, - options: Keyword.merge(transport_opts, @forced_transport_options) + options: Keyword.merge(keyword_options, @forced_transport_options) ++ other_options } {transport, connection_opts} diff --git a/lib/xandra/options_validators.ex b/lib/xandra/options_validators.ex index 31b9ec7d..2aa743bb 100644 --- a/lib/xandra/options_validators.ex +++ b/lib/xandra/options_validators.ex @@ -30,16 +30,35 @@ defmodule Xandra.OptionsValidators do @spec validate_node(term()) :: {:ok, {String.t(), integer()}} | {:error, String.t()} def validate_node(value) when is_binary(value) do - case String.split(value, ":", parts: 2) do - [address, port] -> - case Integer.parse(port) do - {port, ""} when port in 0..65535 -> {:ok, {address, port}} - {port, ""} -> {:error, "invalid port (outside of the 0..65535 range): #{port}"} - _ -> {:error, "invalid node: #{inspect(value)}"} - end - - [address] -> - {:ok, {address, 9042}} + # Remove surrounding square brackets from IPv6 values. + value = value |> String.replace("[", "") |> String.replace("]", "") + + {address, port} = + case String.split(value, ":") do + # FQDN or IPv4. + [address] -> + {address, "9042"} + + # FQDN:PORT or IPv4:PORT. IPv6 addresses have to have at least 2 colons. + [address, port] -> + {address, port} + + # IPv6. + splits -> + # Our IPv6 address can either parse as a valid address or have the + # additional port suffix. + with {:ok, {_, _, _, _, _, _, _, _}} <- + :inet.parse_address(value |> String.to_charlist()) do + {value, "9042"} + else + _ -> {splits |> Enum.drop(-1) |> Enum.join(":"), List.last(splits)} + end + end + + case Integer.parse(port) do + {port, ""} when port in 0..65535 -> {:ok, {address, port}} + {port, ""} -> {:error, "invalid port (outside of the 0..65535 range): #{port}"} + _ -> {:error, "invalid node: #{inspect(value)}"} end end diff --git a/test/xandra/cluster_test.exs b/test/xandra/cluster_test.exs index 47463ed4..abc358a7 100644 --- a/test/xandra/cluster_test.exs +++ b/test/xandra/cluster_test.exs @@ -147,6 +147,19 @@ defmodule Xandra.ClusterTest do Xandra.Cluster.start_link(port: 9042) end end + + test "with the :inet6 transport option" do + assert {:ok, _conn} = Xandra.Cluster.start_link(nodes: ["::1"], transport_options: [:inet6]) + + assert {:ok, _conn} = + Xandra.Cluster.start_link(nodes: ["::1:9042"], transport_options: [:inet6]) + + assert {:ok, _conn} = + Xandra.Cluster.start_link(nodes: ["[::1]"], transport_options: [:inet6]) + + assert {:ok, _conn} = + Xandra.Cluster.start_link(nodes: ["[::1]:9042"], transport_options: [:inet6]) + end end describe "start_link/1" do From 6bfa8073c698598fdf6d61432497dcb40f2ffd06 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Fri, 6 Dec 2024 11:25:33 -0500 Subject: [PATCH 4/4] chore: add tests to ensure that forced transport options are respected --- test/xandra/cluster_test.exs | 10 ++++++++++ test/xandra_test.exs | 13 +++++++++++++ 2 files changed, 23 insertions(+) diff --git a/test/xandra/cluster_test.exs b/test/xandra/cluster_test.exs index abc358a7..453e3f39 100644 --- a/test/xandra/cluster_test.exs +++ b/test/xandra/cluster_test.exs @@ -270,6 +270,16 @@ defmodule Xandra.ClusterTest do assert GenServer.whereis(name) == pid stop_supervised!(name) end + + @tag telemetry_events: [[:xandra, :connected]] + test "successfully connect even with an invalid transport option", + %{base_options: opts, telemetry_ref: telemetry_ref} do + opts = Keyword.merge(opts, transport_options: [active: true]) + _pid = start_link_supervised!({Cluster, opts}) + + # Assert that we already received events. + assert_received {[:xandra, :connected], ^telemetry_ref, %{}, %{}} + end end describe "starting up" do diff --git a/test/xandra_test.exs b/test/xandra_test.exs index dbb251cf..f2f4eb51 100644 --- a/test/xandra_test.exs +++ b/test/xandra_test.exs @@ -195,6 +195,19 @@ defmodule XandraTest do assert_received {[:xandra, :failed_to_connect], ^telemetry_ref, %{}, %{connection: ^conn}} end + + test "invalid transport option gets forcibly overwritten" do + telemetry_ref = + :telemetry_test.attach_event_handlers(self(), [[:xandra, :connected]]) + + # Set a transport option `active: true` that normally would cause the + # connection to fail. This connection should succeed because start_link + # forcibly overrides and sets some necessary options. + options = Keyword.merge(default_start_options(), transport_options: [active: true]) + + conn = start_supervised!({Xandra, options}) + assert_receive {[:xandra, :connected], ^telemetry_ref, %{}, %{connection: ^conn}} + end end describe "execute/3,4" do