Skip to content

Commit

Permalink
Rack handler should use provided default host
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
schneems committed Jan 4, 2019
1 parent e5d566e commit c24c0c8
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 1 deletion.
10 changes: 9 additions & 1 deletion lib/puma/dsl.rb
Expand Up @@ -57,6 +57,14 @@ def _offer_plugins
@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
Expand Down Expand Up @@ -140,7 +148,7 @@ def clear_binds!
# 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

Expand Down
3 changes: 3 additions & 0 deletions lib/rack/handler/puma.rb
Expand Up @@ -49,6 +49,9 @@ def self.config(app, options = {})
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
Expand Down
38 changes: 38 additions & 0 deletions test/test_rack_handler.rb
Expand Up @@ -123,6 +123,44 @@ def test_config_file_wins_over_port
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
Expand Down

0 comments on commit c24c0c8

Please sign in to comment.