From c24c0c883496f581d9092bbe7f7431129eeb7190 Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 4 Jan 2019 13:15:31 -0600 Subject: [PATCH] Rack handler should use provided default host This issue is somewhat tricky. When Rails is booted via `rails server` there are two types of configuration options passed, ones specified directly by a user like `rails s -p 3001` will always "win". For any other config that is not explicitly passed in, puma will consider it a "default". For example when you run `rails s` (without -p) then the default port will be 3000. There is one other way to configure puma though, and that is via a config file: ``` # config/puma.rb port 3002 ``` This is the order of precedence for configuration 1) Anything the user explicitly passes to `rails s` 2) Config specified in `config/puma.rb` file 3) Default values passed in via `rails s` 4) Defaults values stored in puma This fallback mechanism works well except in the case of calling `port` in a `config/puma.rb` file. To understand look at the [old method definition](https://github.com/puma/puma/blob/2668597ec1dd9546d83db9f2ec5ad092add483e6/lib/puma/dsl.rb#L140-L145): ``` def port(port, host=nil) host ||= Configuration::DefaultTCPHost bind "tcp://#{host}:#{port}" end ``` When the `port` method gets called, even if the user did not specify a `host` the `Configuration::DefaultTCPHost` will be used, which is a problem for local development because it defaults to `0.0.0.0`. [SO about 0.0.0.0 versus localhost](https://stackoverflow.com/questions/20778771/what-is-the-difference-between-0-0-0-0-127-0-0-1-and-localhost). In this case, while a user did directly specify a port, they did not specify a host, so you would expect the `rails s` defaults passed in to take affect. To make Puma respect that the host coming from `rails s` has more precedence than it's own default host, we must introduce the ability to set and retrieve a default_host value. This is then used in the rack handler so when `rails s` passes in `:Host => "localhost"` then it is used instead of reverting to `0.0.0.0`. The issue with #1699 is the test was wrong, it would have failed if a config file was present with a `port` invocation. --- lib/puma/dsl.rb | 10 +++++++++- lib/rack/handler/puma.rb | 3 +++ test/test_rack_handler.rb | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/lib/puma/dsl.rb b/lib/puma/dsl.rb index b5bebf43..a342200e 100644 --- a/lib/puma/dsl.rb +++ b/lib/puma/dsl.rb @@ -57,6 +57,14 @@ module Puma @plugins.clear end + def set_default_host(host) + @options[:default_host] = host + end + + def default_host + @options[:default_host] || Configuration::DefaultTCPHost + end + def inject(&blk) instance_eval(&blk) end @@ -140,7 +148,7 @@ module Puma # Define the TCP port to bind to. Use +bind+ for more advanced options. # def port(port, host=nil) - host ||= Configuration::DefaultTCPHost + host ||= default_host bind "tcp://#{host}:#{port}" end diff --git a/lib/rack/handler/puma.rb b/lib/rack/handler/puma.rb index 3ff18958..4607b7c1 100644 --- a/lib/rack/handler/puma.rb +++ b/lib/rack/handler/puma.rb @@ -49,6 +49,9 @@ module Rack self.set_host_port_to_config(host, port, user_config) end + if default_options[:Host] + file_config.set_default_host(default_options[:Host]) + end self.set_host_port_to_config(default_options[:Host], default_options[:Port], default_config) user_config.app app diff --git a/test/test_rack_handler.rb b/test/test_rack_handler.rb index 5007e20e..3f3a6987 100644 --- a/test/test_rack_handler.rb +++ b/test/test_rack_handler.rb @@ -123,6 +123,44 @@ class TestUserSuppliedOptionsIsEmpty < Minitest::Test end end end + + def test_default_host_when_using_config_file + user_port = 5001 + file_port = 6001 + + Dir.mktmpdir do |d| + Dir.chdir(d) do + FileUtils.mkdir("config") + File.open("config/puma.rb", "w") { |f| f << "port #{file_port}" } + + @options[:Host] = "localhost" + @options[:Port] = user_port + conf = Rack::Handler::Puma.config(->{}, @options) + conf.load + + assert_equal ["tcp://localhost:#{file_port}"], conf.options[:binds] + end + end + end + + def test_default_host_when_using_config_file_with_explicit_host + user_port = 5001 + file_port = 6001 + + Dir.mktmpdir do |d| + Dir.chdir(d) do + FileUtils.mkdir("config") + File.open("config/puma.rb", "w") { |f| f << "port #{file_port}, '1.2.3.4'" } + + @options[:Host] = "localhost" + @options[:Port] = user_port + conf = Rack::Handler::Puma.config(->{}, @options) + conf.load + + assert_equal ["tcp://1.2.3.4:#{file_port}"], conf.options[:binds] + end + end + end end class TestUserSuppliedOptionsIsNotPresent < Minitest::Test