-
-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support IPv6 nodes and transport options #374
base: main
Are you sure you want to change the base?
Conversation
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.
… update validate_node to support IPv6 nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, especially when it comes to non-keyword-value transport options. Left a couple of comments but this is already looking great!
@@ -444,7 +444,7 @@ defmodule Xandra do | |||
""" | |||
], | |||
transport_options: [ | |||
type: :keyword_list, | |||
type: {:or, [:keyword_list, {:list, :any}]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{:list, :any}
is a superset of :keyword_list
. Maybe we can go with this instead:
type: {:or, [:keyword_list, {:list, :any}]}, | |
type: {:list, {:or, [{:tuple, [:atom, :any]}, :atom]}}, |
or just go with {:list, :any}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the :gen_tcp
options support additionally the inet_family
atoms as well as lists and binaries. I don't have any examples of what those might be, but, if you're okay with it, we could start with {:list, :any}
.
option() =
{active, true | false | once | -32768..32767} |
...
{low_watermark, integer() >= 0} |
{mode, list | binary} |
list |
binary |
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sure let's go with tat.
# Construct the transport options. | ||
{keyword_options, other_options} = | ||
Enum.split_with(transport_opts, fn x -> Keyword.keyword?([x]) end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do this. Do:
options: @forced_transport_options ++ transport_opts
as putting the forced options at the beginning overrides later options. Please double check this but I’m pretty sure 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a conflicting transport option here: {:active, true}
which causes the connection to fail.
I tried both flipped configurations to see how ordering affected option precedence, but in both cases the connection failed.
options: @forced_transport_options ++ transport_opts
and
options: transport_opts ++ @forced_transport_options
{:ok, conn} = Xandra.Cluster.start_link(nodes: ["my-scylla-cluster.internal"], transport_options: [:inet6, {:active, false}], authentication: {Xandra.Authenticator.Password, [username: System.get_env("SCYLLA_USERNAME"), password: System.get_env("SCYLLA_PASSWORD")]}, sync_connect: 1000)
IO.inspect(@forced_transport_options ++ transport_opts)
IO.inspect(transport_opts ++ @forced_transport_options)
# [{:packet, :raw}, {:mode, :binary}, {:active, false}, :inet6, {:active, true}]
# [:inet6, {:active, true}, {:packet, :raw}, {:mode, :binary}, {:active, false}]
@@ -409,16 +409,21 @@ defmodule Xandra.Connection do | |||
nil -> data.original_options | |||
end | |||
|
|||
# Construct the transport options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, but you can use proplists
:
transport_options = Keyword.get(options, :transport_options, [])
buffer = :proplists.get_value(transport_options, :buffer, @default_transport_buffer_size)
transport_options = [buffer: buffer] ++ @forced_transport_options ++ :proplists.delete(:buffer, transport_options)
Should work but please double check 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ran into the same issue as in the other comment where I could override the @forced_transport_options
unexpectedly with {:active, true}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant to both of these comments, I've added tests to check if the forced transport opts are being respected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erhlee-bird mh I’m confused:
iex> {:ok, socket} = :gen_tcp.connect(~c"google.com", 80, [{:active, true}, {:active, false}])
{:ok, #Port<0.4>}
iex> :inet.getopts(socket, [:active])
{:ok, [active: false]}
Seems like overriding works here?
|
||
[address] -> | ||
{:ok, {address, 9042}} | ||
# Remove surrounding square brackets from IPv6 values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this logic, we could "pop" out the port like this, regardless of the type of IP using Regex.split/2
with lookahead:
case Regex.split(~r/:(?=\d+$)/, value) do
[address, port] ->
# Same as before
[address] ->
# Also same as before
end
This way we don't have to parse the IPv6 address, we can just leave what the user passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about doing that, but there's ambiguity if you have IPv6 addresses without a port. The first 2 examples below using the Regex method above parse the address incorrectly.
iex(3)> Xandra.OptionsValidators.validate_node("::1")
{:ok, {":", 1}}
iex(4)> Xandra.OptionsValidators.validate_node("2001:db8::1")
{:ok, {"2001:db8:", 1}}
iex(5)> Xandra.OptionsValidators.validate_node("[2001:db8::1]:9042")
{:ok, {"2001:db8::1", 9042}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works with a lookbehind assertion:
iex(2)> Regex.split(~r/(?<!:):(?=\d+$)/, "::1")
["::1"]
iex(3)> Regex.split(~r/(?<!:):(?=\d+$)/, "2001:db8::1")
["2001:db8::1"]
iex(5)> Regex.split(~r/(?<!:):(?=\d+$)/, "[2001:db8::1]:9042")
["[2001:db8::1]", "9042"]
(need to then address |> String.trim_leading("[") |> String.trim_trailing("]")
)
Hi there, thanks for the awesome library!
I'm running infrastructure on a platform called fly.io which runs IPv6 private networks.
I was unable to connect to my ScyllaDB cluster using Xandra and found that there were a few limitations to IPv6 support.
This pull request adds proposed fixes for the below 2 issues. I've added some tests and tried to ensure that this change would not break
Xandra.start_link()
andXandra.Cluster.start_link()
but there may be other places using these options that I may have missed.In my fork, I'm now able to establish an IPv6 connection to my cluster and get kicked back due to lack of authentication whereas before connections would hang or hit sync_connection_timeout errors.
Cannot specify IPv6 node addresses.
Cannot pass
:inet6
as a transport option for:gen_tcp
:gen_tcp
accepts options that are not strictly keywords.