Repository: elixir-grpc/grpc (elixir)

Issue:
Unable to specify gun tcp_opts field when using gun v2
**Describe the bug**
When passing options for gun via the `:adapter_opts` key in the 3rd argument to the function `GRPC.Stub.connect/3`, if you specify the `tcp_opts` field (which should be of type `[gen_tcp:connect_option()]`, see: https://ninenines.eu/docs/en/gun/2.0/manual/gun/) the field is always replaced with the value of `@default_transport_opts`. 

**To Reproduce**
Ensure you are using gun v2 (you should be unless you specify a different version with `override: true` in your `mix.exs`) and make sure the `gun` application is loaded. Add some logging to `GRPC.Stub.connect_insecurely/2` or trace `:gun.open/3` using e.g. `recon_trace:calls`, and then call `GRPC.Stub.connect(my_addr, my_port, [adapter_opts: %{tcp_opts: my_tcp_opts}])` where `my_tcp_opts` is a proplist containing some options you want to give to gun's `tcp_opts` field.

**Expected behavior**
The options passed to `gun:open/3` should include the values you provided for the `tcp_opts` field. Instead you will find that you can only ever get the default tcp_opts value provided by the module attribute `@default_transport_opts` in the `GRPC.Adapter.Gun` module

**Logs**
```elixir
Interactive Elixir (1.10.1) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> :code.ensure_loaded(:gun)
{:module, :gun}
iex(2)> :recon_trace.calls({:gun, :open, :_}, 1)
2
iex(3)> grpc_opts = [adapter_opts: %{tcp_opts: [port: 6969, nodelay: true]}]
[adapter_opts: %{tcp_opts: [port: 6969, nodelay: true]}]
iex(4)> GRPC.Stub.connect("localhost", 50051, grpc_opts)
14:26:13.021314 <0.250.0> gun:open("localhost", 50051, #{http2_opts=>#{settings_timeout=>infinity}, protocols=>[http2], retry=>100, retry_fun=>fun 'Elixir.GRPC.Adapter.Gun':retry_fun/2, tcp_opts=>[{nodelay,true}], transport=>tcp})
Recon tracer rate limit tripped.
```

**Versions:**
- OS: NixOS 20.03
- Erlang: Erlang/OTP 22 [erts-10.5.5]
- Elixir: 1.10.1
- mix.lock(grpc, gun, cowboy, cowlib): 
 ```elixir
"grpc": {:git, "https://github.com/elixir-grpc/grpc.git", "9b8bfce9ba529805ac0bd6d183bf9563a9f24b0c", []}
 "cowboy": {:hex, :cowboy, "2.7.0", "91ed100138a764355f43316b1d23d7ff6bdb0de4ea618cb5d8677c93a7a2f115", [:rebar3], [{:cowlib, "~> 2.8.0", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "~> 1.7.1", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "04fd8c6a39edc6aaa9c26123009200fc61f92a3a94f3178c527b70b767c6e605"},
  "cowlib": {:hex, :cowlib, "2.9.1", "61a6c7c50cf07fdd24b2f45b89500bb93b6686579b069a89f88cb211e1125c78", [:rebar3], [], "hexpm", "e4175dc240a70d996156160891e1c62238ede1729e45740bdd38064dad476170"},
  "gun": {:hex, :grpc_gun, "2.0.0", "f99678a2ab975e74372a756c86ec30a8384d3ac8a8b86c7ed6243ef4e61d2729", [:rebar3], [{:cowlib, "~> 2.8.0", [hex: :cowlib, repo: "hexpm", optional: false]}], "hexpm", "03dbbca1a9c604a0267a40ea1d69986225091acb822de0b2dbea21d5815e410b"}
```

**Additional context**
The problem is that on lines [27](https://github.com/elixir-grpc/grpc/blob/master/lib/grpc/adapter/gun.ex#L27) and [45](https://github.com/elixir-grpc/grpc/blob/master/lib/grpc/adapter/gun.ex#L45) of `GRPC.Adapter.Gun`, the call to `Map.merge/2` always replaces the value of `tcp_opts` in the first argument (which is the value supplied by the user) with the value in the second argument (the default value). 

Note that things work as expected if the user is using gun v1 and instead uses the key `transport_opts`. Do we really need the checks to the version of gun here? I think it makes sense to just pass through whichever options the user supplies, since gun itself will validate the options when `gun:open` is called anyway.




Pull request:
fix: avoid overriding user config if possible
closes #176 

Patch:
diff --git a/lib/grpc/client/adapters/gun.ex b/lib/grpc/client/adapters/gun.ex
index 827cb76..1dd6de3 100644
--- a/lib/grpc/client/adapters/gun.ex
+++ b/lib/grpc/client/adapters/gun.ex
@@ -1,6 +1,6 @@
 defmodule GRPC.Client.Adapters.Gun do
   @moduledoc """
-  A client (`b:GRPC.Client.Adapter`) adapter using `:gun`.
+  A client adapter using Gun
 
   `conn_pid` and `stream_ref` are stored in `GRPC.Server.Stream`.
   """
@@ -8,55 +8,49 @@ defmodule GRPC.Client.Adapters.Gun do
   @behaviour GRPC.Client.Adapter
 
   @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 =
-      if gun_v2?() do
-        Map.put(open_opts, :tls_opts, transport_opts)
-      else
-        Map.put(open_opts, :transport_opts, 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
 
   defp connect_insecurely(channel, opts) do
-    opts = Map.update(opts, :http2_opts, @default_http2_opts, &Map.merge(&1, @default_http2_opts))
+    opts =
+      Map.update(
+        opts,
+        :http2_opts,
+        %{settings_timeout: :infinity},
+        &Map.put(&1, :settings_timeout, :infinity)
+      )
 
-    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 =
-      if gun_v2?() do
-        Map.put(open_opts, :tcp_opts, transport_opts)
-      else
-        Map.put(open_opts, :transport_opts, 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)
 
@@ -267,17 +261,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/lib/grpc/credential.ex b/lib/grpc/credential.ex
index 17fc5c9..1df78ea 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])
diff --git a/lib/grpc/stub.ex b/lib/grpc/stub.ex
index 78253ae..eb5c7f0 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
diff --git a/mix.exs b/mix.exs
index 7b9f3a9..af6eed0 100644
--- a/mix.exs
+++ b/mix.exs
@@ -45,8 +45,8 @@ defmodule GRPC.Mixfile do
       {:gun, "~> 2.0.1", hex: :grpc_gun},
       {:cowlib, "~> 2.11"},
       {:protobuf, "~> 0.10", only: [:dev, :test]},
-      {:ex_doc, "~> 0.28", only: :dev},
-      {:dialyxir, "~> 1.1", only: [:dev, :test], runtime: false}
+      {:ex_doc, "~> 0.28.0", only: :dev},
+      {:dialyxir, "~> 1.1.0", only: [:dev, :test], runtime: false}
     ]
   end
 


Test patch:
diff --git a/test/grpc/adapter/gun_test.exs b/test/grpc/adapter/gun_test.exs
new file mode 100644
index 0000000..04840eb
--- /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/support/factory.ex b/test/support/factory.ex
new file mode 100644
index 0000000..fceaae0
--- /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