Fix config options merging (#111)

* Fix config options merging * Fixup existing tests * Add test coverage --------- Co-authored-by: Mat Trudel <[email protected]>
mtrudel · Feb 23, 2024 · 1116dba · 1116dba
1 parent 9a4b313
commit 1116dba
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 17 deletions.
diff --git a/lib/thousand_island/transports/ssl.ex b/lib/thousand_island/transports/ssl.ex
@@ -57,7 +57,10 @@ defmodule ThousandIsland.Transports.SSL do
       reuseaddr: true
     ]
 
-    resolved_options = @hardcoded_options ++ user_options ++ default_options
+    resolved_options =
+      default_options
+      |> Keyword.merge(user_options)
+      |> Keyword.merge(@hardcoded_options)
 
     if not Enum.any?(
          [:keyfile, :key, :sni_hosts, :sni_fun],

diff --git a/lib/thousand_island/transports/tcp.ex b/lib/thousand_island/transports/tcp.ex
@@ -55,7 +55,11 @@ defmodule ThousandIsland.Transports.TCP do
       reuseaddr: true
     ]
 
-    resolved_options = @hardcoded_options ++ user_options ++ default_options
+    resolved_options =
+      default_options
+      |> Keyword.merge(user_options)
+      |> Keyword.merge(@hardcoded_options)
+
     :gen_tcp.listen(port, resolved_options)
   end
 

diff --git a/test/thousand_island/server_test.exs b/test/thousand_island/server_test.exs
@@ -35,6 +35,17 @@ defmodule ThousandIsland.ServerTest do
     end
   end
 
+  defmodule ReadOpt do
+    use ThousandIsland.Handler
+
+    @impl ThousandIsland.Handler
+    def handle_data(data, socket, state) do
+      opts = [String.to_atom(data)]
+      ThousandIsland.Socket.send(socket, inspect(ThousandIsland.Socket.getopts(socket, opts)))
+      {:close, state}
+    end
+  end
+
   defmodule Error do
     use ThousandIsland.Handler
 
@@ -204,7 +215,7 @@ defmodule ThousandIsland.ServerTest do
     assert {:ok, []} == ThousandIsland.connection_pids(server_pid)
   end
 
-  describe "suspend / reume" do
+  describe "suspend / resume" do
     test "suspend should stop accepting connections but keep existing ones open" do
       {:ok, server_pid, port} = start_handler(LongEcho, port: 9999)
       {:ok, client} = :gen_tcp.connect(:localhost, port, active: false)
@@ -364,23 +375,79 @@ defmodule ThousandIsland.ServerTest do
     end
   end
 
+  describe "configuration" do
+    test "tcp should allow default options to be overridden" do
+      {:ok, _, port} = start_handler(ReadOpt, transport_options: [send_timeout: 1230])
+      {:ok, client} = :gen_tcp.connect(:localhost, port, active: false)
+      :gen_tcp.send(client, "send_timeout")
+      {:ok, ~c"{:ok, [send_timeout: 1230]}"} = :gen_tcp.recv(client, 0, 100)
+    end
+
+    test "tcp should not allow hardcoded options to be overridden" do
+      {:ok, _, port} = start_handler(ReadOpt, transport_options: [mode: :list])
+      {:ok, client} = :gen_tcp.connect(:localhost, port, active: false)
+      :gen_tcp.send(client, "mode")
+      {:ok, ~c"{:ok, [mode: :binary]}"} = :gen_tcp.recv(client, 0, 100)
+    end
+
+    test "ssl should allow default options to be overridden" do
+      {:ok, _, port} =
+        start_handler(ReadOpt,
+          transport_module: ThousandIsland.Transports.SSL,
+          transport_options: [
+            send_timeout: 1230,
+            certfile: Path.join(__DIR__, "../support/cert.pem"),
+            keyfile: Path.join(__DIR__, "../support/key.pem")
+          ]
+        )
+
+      {:ok, client} =
+        :ssl.connect(:localhost, port,
+          active: false,
+          verify: :verify_none,
+          cacertfile: Path.join(__DIR__, "../support/ca.pem")
+        )
+
+      :ssl.send(client, "send_timeout")
+      {:ok, ~c"{:ok, [send_timeout: 1230]}"} = :ssl.recv(client, 0, 100)
+    end
+
+    test "ssl should not allow hardcoded options to be overridden" do
+      {:ok, _, port} =
+        start_handler(ReadOpt,
+          transport_module: ThousandIsland.Transports.SSL,
+          transport_options: [
+            mode: :list,
+            certfile: Path.join(__DIR__, "../support/cert.pem"),
+            keyfile: Path.join(__DIR__, "../support/key.pem")
+          ]
+        )
+
+      {:ok, client} =
+        :ssl.connect(:localhost, port,
+          active: false,
+          verify: :verify_none,
+          cacertfile: Path.join(__DIR__, "../support/ca.pem")
+        )
+
+      :ssl.send(client, "mode")
+      {:ok, ~c"{:ok, [mode: :binary]}"} = :ssl.recv(client, 0, 100)
+    end
+  end
+
   describe "invalid configuration" do
     @tag capture_log: true
     test "it should error if a certificate is not found" do
-      server_args = [
-        port: 0,
-        handler_module: Error,
-        handler_options: [test_pid: self()],
-        transport_module: ThousandIsland.Transports.SSL,
-        transport_options: [
-          certfile: Path.join(__DIR__, "./not/a/cert.pem"),
-          keyfile: Path.join(__DIR__, "./not/a/key.pem"),
-          alpn_preferred_protocols: ["foo"]
-        ]
-      ]
-
-      {:ok, server_pid} = start_supervised({ThousandIsland, server_args})
-      {:ok, {_ip, port}} = ThousandIsland.listener_info(server_pid)
+      {:ok, server_pid, port} =
+        start_handler(Error,
+          handler_options: [test_pid: self()],
+          transport_module: ThousandIsland.Transports.SSL,
+          transport_options: [
+            certfile: Path.join(__DIR__, "./not/a/cert.pem"),
+            keyfile: Path.join(__DIR__, "./not/a/key.pem"),
+            alpn_preferred_protocols: ["foo"]
+          ]
+        )
 
       {:error, _} =
         :ssl.connect(~c"localhost", port,