From b109aad4a0431950e5dc18634b3b7ae8956ea6f3 Mon Sep 17 00:00:00 2001 From: Paulo Valente <16843419+polvalente@users.noreply.github.com> Date: Tue, 19 Jul 2022 13:12:10 -0300 Subject: [PATCH 1/5] fix: avoid overriding user config if possible --- lib/grpc/adapter/gun.ex | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/grpc/adapter/gun.ex b/lib/grpc/adapter/gun.ex index bd7234aa..d751d455 100644 --- a/lib/grpc/adapter/gun.ex +++ b/lib/grpc/adapter/gun.ex @@ -14,15 +14,13 @@ defmodule GRPC.Adapter.Gun do def connect(channel, opts), do: connect_insecurely(channel, opts) defp connect_securely(%{cred: %{ssl: ssl}} = channel, opts) do - transport_opts = Map.get(opts, :transport_opts, @default_transport_opts ++ ssl) + transport_opts = Map.get(opts, :transport_opts) || @default_transport_opts ++ ssl open_opts = %{transport: :ssl, protocols: [:http2]} open_opts = - if gun_v2?() do - Map.put(open_opts, :tls_opts, transport_opts) - else - Map.put(open_opts, :transport_opts, transport_opts) - end + Map.update(open_opts, :tls_opts, transport_opts, fn current -> + Keyword.merge(current, transport_opts) + end) open_opts = Map.merge(opts, open_opts) @@ -32,15 +30,13 @@ defmodule GRPC.Adapter.Gun do defp connect_insecurely(channel, opts) do opts = Map.update(opts, :http2_opts, @default_http2_opts, &Map.merge(&1, @default_http2_opts)) - transport_opts = Map.get(opts, :transport_opts, @default_transport_opts) + transport_opts = Map.get(opts, :transport_opts) || @default_transport_opts open_opts = %{transport: :tcp, protocols: [:http2]} open_opts = - if gun_v2?() do - Map.put(open_opts, :tcp_opts, transport_opts) - else - Map.put(open_opts, :transport_opts, transport_opts) - end + Map.update(open_opts, :tls_opts, transport_opts, fn current -> + Keyword.merge(current, transport_opts) + end) open_opts = Map.merge(opts, open_opts) From 467882ceea7c059cf50256431b6b1ad54c4df435 Mon Sep 17 00:00:00 2001 From: Paulo Valente <16843419+polvalente@users.noreply.github.com> Date: Fri, 22 Jul 2022 20:10:48 -0300 Subject: [PATCH 2/5] feat: make transport option replacement work --- lib/grpc/client/adapters/gun.ex | 53 +++++--------- mix.exs | 6 +- test/grpc/adapter/gun_test.exs | 87 +++++++++++++++++++++++ test/grpc/integration/connection_test.exs | 8 --- test/support/factory.ex | 52 ++++++++++++++ test/support/feature_server.ex | 7 ++ 6 files changed, 167 insertions(+), 46 deletions(-) create mode 100644 test/grpc/adapter/gun_test.exs create mode 100644 test/support/factory.ex create mode 100644 test/support/feature_server.ex diff --git a/lib/grpc/client/adapters/gun.ex b/lib/grpc/client/adapters/gun.ex index b7c00ef7..d6592b29 100644 --- a/lib/grpc/client/adapters/gun.ex +++ b/lib/grpc/client/adapters/gun.ex @@ -1,30 +1,30 @@ defmodule GRPC.Client.Adapters.Gun do - @moduledoc false + @moduledoc """ + A client adapter using Gun + """ @behaviour GRPC.Client.Adapter - # A client adapter using Gun. - # conn_pid and stream_ref is stored in `GRPC.Server.Stream`. + # conn_pid and stream_ref are stored in `GRPC.Server.Stream` @default_transport_opts [nodelay: true] @default_http2_opts %{settings_timeout: :infinity} @max_retries 100 @impl true - def connect(channel, nil), do: connect(channel, %{}) + def connect(channel, opts \\ %{}) def connect(%{scheme: "https"} = channel, opts), do: connect_securely(channel, opts) def connect(channel, opts), do: connect_insecurely(channel, opts) defp connect_securely(%{cred: %{ssl: ssl}} = channel, opts) do - transport_opts = Map.get(opts, :transport_opts) || @default_transport_opts ++ ssl - open_opts = %{transport: :ssl, protocols: [:http2]} + transport_opts = Map.get(opts, :transport_opts) || [] - open_opts = - Map.update(open_opts, :tls_opts, transport_opts, fn current -> - Keyword.merge(current, transport_opts) - end) + tls_opts = Keyword.merge(@default_transport_opts ++ ssl, transport_opts) - open_opts = Map.merge(opts, open_opts) + open_opts = + opts + |> Map.delete(:transport_opts) + |> Map.merge(%{transport: :ssl, protocols: [:http2], tls_opts: tls_opts}) do_connect(channel, open_opts) end @@ -32,26 +32,20 @@ defmodule GRPC.Client.Adapters.Gun do defp connect_insecurely(channel, opts) do opts = Map.update(opts, :http2_opts, @default_http2_opts, &Map.merge(&1, @default_http2_opts)) - transport_opts = Map.get(opts, :transport_opts) || @default_transport_opts - open_opts = %{transport: :tcp, protocols: [:http2]} + transport_opts = Map.get(opts, :transport_opts) || [] - open_opts = - Map.update(open_opts, :tls_opts, transport_opts, fn current -> - Keyword.merge(current, transport_opts) - end) + tcp_opts = Keyword.merge(@default_transport_opts, transport_opts) - open_opts = Map.merge(opts, open_opts) + open_opts = + opts + |> Map.delete(:transport_opts) + |> Map.merge(%{transport: :tcp, protocols: [:http2], tcp_opts: tcp_opts}) do_connect(channel, open_opts) end defp do_connect(%{host: host, port: port} = channel, open_opts) do - open_opts = - if gun_v2?() do - Map.merge(%{retry: @max_retries, retry_fun: &__MODULE__.retry_fun/2}, open_opts) - else - open_opts - end + open_opts = Map.merge(%{retry: @max_retries, retry_fun: &__MODULE__.retry_fun/2}, open_opts) {:ok, conn_pid} = open(host, port, open_opts) @@ -262,17 +256,6 @@ defmodule GRPC.Client.Adapters.Gun do end end - @char_2 List.first('2') - def gun_v2?() do - case :application.get_key(:gun, :vsn) do - {:ok, [@char_2 | _]} -> - true - - _ -> - false - end - end - def retry_fun(retries, _opts) do curr = @max_retries - retries + 1 diff --git a/mix.exs b/mix.exs index 86644f50..b1580b9b 100644 --- a/mix.exs +++ b/mix.exs @@ -40,9 +40,9 @@ defmodule GRPC.Mixfile do {:gun, "~> 2.0.0-rc.2"}, {:cowlib, "~> 2.11"}, {:protobuf, "~> 0.10", only: [:dev, :test]}, - {:ex_doc, "~> 0.28", only: :dev}, - {:inch_ex, "~> 2.0", only: [:dev, :test]}, - {:dialyxir, "~> 1.1", only: [:dev, :test], runtime: false} + {:ex_doc, "~> 0.28.0", only: :dev}, + {:inch_ex, "~> 2.0.0", only: [:dev, :test]}, + {:dialyxir, "~> 1.1.0", only: [:dev, :test], runtime: false} ] end diff --git a/test/grpc/adapter/gun_test.exs b/test/grpc/adapter/gun_test.exs new file mode 100644 index 00000000..04840eb5 --- /dev/null +++ b/test/grpc/adapter/gun_test.exs @@ -0,0 +1,87 @@ +defmodule GRPC.Client.Adapters.GunTest do + use ExUnit.Case, async: true + + import GRPC.Factory + + alias GRPC.Client.Adapters.Gun + + describe "connect/2" do + setup do + server_credential = build(:credential) + {:ok, _, port} = GRPC.Server.start(FeatureServer, 0, cred: server_credential) + + on_exit(fn -> + :ok = GRPC.Server.stop(FeatureServer) + end) + + %{ + port: port, + credential: + build(:credential, + ssl: Keyword.take(server_credential.ssl, [:certfile, :keyfile, :versions]) + ) + } + end + + test "connects insecurely (default options)", %{port: port, credential: credential} do + channel = build(:channel, port: port, host: "localhost", cred: credential) + + assert {:ok, result} = Gun.connect(channel) + + assert %{channel | adapter_payload: %{conn_pid: result.adapter_payload.conn_pid}} == result + end + + test "connects insecurely (custom options)", %{port: port, credential: credential} do + channel = build(:channel, port: port, host: "localhost", cred: credential) + + # Ensure that it works + assert {:ok, result} = Gun.connect(channel, %{transport_opts: [ip: :loopback]}) + assert %{channel | adapter_payload: %{conn_pid: result.adapter_payload.conn_pid}} == result + + # Ensure that changing one of the options breaks things + assert {:error, {:down, :badarg}} == + Gun.connect(channel, %{transport_opts: [ip: "256.0.0.0"]}) + end + + test "connects securely (default options)", %{port: port, credential: credential} do + channel = + build(:channel, + port: port, + scheme: "https", + host: "localhost", + cred: credential + ) + + assert {:ok, result} = Gun.connect(channel, %{tls_opts: channel.cred.ssl}) + + assert %{channel | adapter_payload: %{conn_pid: result.adapter_payload.conn_pid}} == result + end + + test "connects securely (custom options)", %{port: port, credential: credential} do + channel = + build(:channel, + port: port, + scheme: "https", + host: "localhost", + cred: credential + ) + + # Ensure that it works + assert {:ok, result} = + Gun.connect(channel, %{ + transport_opts: [certfile: credential.ssl[:certfile], ip: :loopback] + }) + + assert %{channel | adapter_payload: %{conn_pid: result.adapter_payload.conn_pid}} == result + + # Ensure that changing one of the options breaks things + assert {:error, :timeout} == + Gun.connect(channel, %{ + transport_opts: [ + certfile: credential.ssl[:certfile] <> "invalidsuffix", + ip: :loopback + ] + }) + end + end +end diff --git a/test/grpc/integration/connection_test.exs b/test/grpc/integration/connection_test.exs index 2416b778..bdd25cfc 100644 --- a/test/grpc/integration/connection_test.exs +++ b/test/grpc/integration/connection_test.exs @@ -5,14 +5,6 @@ defmodule GRPC.Integration.ConnectionTest do @key_path Path.expand("./tls/server1.key", :code.priv_dir(:grpc)) @ca_path Path.expand("./tls/ca.pem", :code.priv_dir(:grpc)) - defmodule FeatureServer do - use GRPC.Server, service: Routeguide.RouteGuide.Service - - def get_feature(point, _stream) do - Routeguide.Feature.new(location: point, name: "#{point.latitude},#{point.longitude}") - end - end - test "reconnection works" do server = FeatureServer {:ok, _, port} = GRPC.Server.start(server, 0) diff --git a/test/support/factory.ex b/test/support/factory.ex new file mode 100644 index 00000000..fceaae03 --- /dev/null +++ b/test/support/factory.ex @@ -0,0 +1,52 @@ +defmodule GRPC.Factory do + @moduledoc false + + alias GRPC.Channel + alias GRPC.Credential + + @cert_path Path.expand("./tls/server1.pem", :code.priv_dir(:grpc)) + @key_path Path.expand("./tls/server1.key", :code.priv_dir(:grpc)) + @ca_path Path.expand("./tls/ca.pem", :code.priv_dir(:grpc)) + + def build(resource, attrs \\ %{}) do + name = :"#{resource}_factory" + + data = + if function_exported?(__MODULE__, name, 1) do + apply(__MODULE__, name, [attrs]) + else + apply(__MODULE__, name, []) + end + + Map.merge(data, Map.new(attrs)) + end + + def channel_factory do + %Channel{ + host: "localhost", + port: 1337, + scheme: "http", + cred: build(:credential), + adapter: GRPC.Client.Adapters.Gun, + adapter_payload: %{}, + codec: GRPC.Codec.Proto, + interceptors: [], + compressor: nil, + accepted_compressors: [], + headers: [] + } + end + + def credential_factory do + %Credential{ + ssl: [ + certfile: @cert_path, + cacertfile: @ca_path, + keyfile: @key_path, + verify: :verify_peer, + fail_if_no_peer_cert: true, + versions: [:"tlsv1.2"] + ] + } + end +end diff --git a/test/support/feature_server.ex b/test/support/feature_server.ex new file mode 100644 index 00000000..3951ea8b --- /dev/null +++ b/test/support/feature_server.ex @@ -0,0 +1,7 @@ +defmodule FeatureServer do + use GRPC.Server, service: Routeguide.RouteGuide.Service + + def get_feature(point, _stream) do + Routeguide.Feature.new(location: point, name: "#{point.latitude},#{point.longitude}") + end +end From 45d41471613787710f2fc98d001366ad96164a55 Mon Sep 17 00:00:00 2001 From: Paulo Valente <16843419+polvalente@users.noreply.github.com> Date: Fri, 22 Jul 2022 20:20:42 -0300 Subject: [PATCH 3/5] fix: || %{} --- lib/grpc/client/adapters/gun.ex | 4 +++- lib/grpc/stub.ex | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/grpc/client/adapters/gun.ex b/lib/grpc/client/adapters/gun.ex index d6592b29..6e951370 100644 --- a/lib/grpc/client/adapters/gun.ex +++ b/lib/grpc/client/adapters/gun.ex @@ -30,7 +30,9 @@ defmodule GRPC.Client.Adapters.Gun do end defp connect_insecurely(channel, opts) do - opts = Map.update(opts, :http2_opts, @default_http2_opts, &Map.merge(&1, @default_http2_opts)) + opts = Map.get(opts, :http2_opts) || @default_http2_opts + + opts = Map.merge(@default_http2_opts, opts) transport_opts = Map.get(opts, :transport_opts) || [] diff --git a/lib/grpc/stub.ex b/lib/grpc/stub.ex index 78253ae5..eb5c7f09 100644 --- a/lib/grpc/stub.ex +++ b/lib/grpc/stub.ex @@ -174,7 +174,7 @@ defmodule GRPC.Stub do accepted_compressors: accepted_compressors, headers: headers } - |> adapter.connect(opts[:adapter_opts]) + |> adapter.connect(opts[:adapter_opts] || %{}) end def retry_timeout(curr) when curr < 11 do From 834665a051c715bdd645eebbb6e9fdedaf7579a9 Mon Sep 17 00:00:00 2001 From: Paulo Valente <16843419+polvalente@users.noreply.github.com> Date: Fri, 22 Jul 2022 20:31:08 -0300 Subject: [PATCH 4/5] fix: override correctly --- lib/grpc/client/adapters/gun.ex | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/grpc/client/adapters/gun.ex b/lib/grpc/client/adapters/gun.ex index 6e951370..b08b61e4 100644 --- a/lib/grpc/client/adapters/gun.ex +++ b/lib/grpc/client/adapters/gun.ex @@ -8,7 +8,6 @@ defmodule GRPC.Client.Adapters.Gun do # conn_pid and stream_ref are stored in `GRPC.Server.Stream` @default_transport_opts [nodelay: true] - @default_http2_opts %{settings_timeout: :infinity} @max_retries 100 @impl true @@ -30,9 +29,13 @@ defmodule GRPC.Client.Adapters.Gun do end defp connect_insecurely(channel, opts) do - opts = Map.get(opts, :http2_opts) || @default_http2_opts - - opts = Map.merge(@default_http2_opts, opts) + opts = + Map.update( + opts, + :http2_opts, + %{settings_timeout: :infinity}, + &Map.put(&1, :settings_timeout, :infinity) + ) transport_opts = Map.get(opts, :transport_opts) || [] From 38d717408f3617cef02f7e84acc923a149f5080f Mon Sep 17 00:00:00 2001 From: Paulo Valente <16843419+polvalente@users.noreply.github.com> Date: Fri, 29 Jul 2022 09:28:38 -0300 Subject: [PATCH 5/5] docs: improve credential docs --- lib/grpc/credential.ex | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/grpc/credential.ex b/lib/grpc/credential.ex index 17fc5c99..1df78ea7 100644 --- a/lib/grpc/credential.ex +++ b/lib/grpc/credential.ex @@ -1,8 +1,15 @@ defmodule GRPC.Credential do @moduledoc """ - Stores credentials for authentication. It can be used to establish secure connections + Stores credentials for authentication. + + It can be used to establish secure connections by passed to `GRPC.Stub.connect/2` as an argument. + Some client and server adapter implementations may + choose to let request options override some of the + configuration here, but this is left as a choice + for each adapter. + ## Examples iex> cred = GRPC.Credential.new(ssl: [cacertfile: ca_path])