Fix nginx duplicate server name warnings in certain cases.

If the api-umbrella.yml config manually defined a wildcard host, then
two hosts could end up defined for the wildcard server name ("_").

This cleans up the implementation a bit, and prevents this duplicate
server name warning. This doesn't actually change the existing behavior,
so there's still some certain configuration situations that might not
really make sense (non-default wildcards), but we'll keep everything
defined as-is to match the hosts config.

src/api-umbrella/cli/read_config.lua

@@ -202,14 +202,23 @@ local function set_computed_config()
     end
   end
 
-  -- Add a default fallback host that will match any hostname, but doesn't
-  -- include any host-specific settings in nginx (like rewrites). This host can
-  -- still then be used to match APIs for unknown hosts.
-  table.insert(config["hosts"], {
-    hostname = "*",
-    _nginx_server_name = "_",
-    default = (not default_host_exists),
-  })
+  -- If a default host hasn't been explicitly defined, then add a default
+  -- fallback host that will match any hostname (but doesn't include any
+  -- host-specific settings in nginx, like rewrites). A default host is
+  -- necessary so nginx handles all hostnames, allowing APIs to be matched for
+  -- hosts that are only defined in the API backend configuration.
+  if not default_host_exists then
+    table.insert(config["hosts"], {
+      hostname = "*",
+      -- Use a slightly different nginx server name to avoid any conflicts with
+      -- explicitly defined wildcard hosts (but aren't the default, which
+      -- doesn't seem particularly likely). There's nothing actually special
+      -- about "_" in nginx, it's just a hostname that won't match anything
+      -- real.
+      _nginx_server_name = "__",
+      default = true,
+    })
+  end
 
   local default_hostname
   if config["hosts"] then

templates/etc/nginx/router.conf.mustache

@@ -221,12 +221,10 @@ http {
   server {
     {{#listen.addresses}}
       listen {{.}}:{{http_port}}{{#default}} default_server so_keepalive=on{{/default}};
+      listen {{.}}:{{https_port}} ssl{{#default}} default_server so_keepalive=on{{/default}};
     {{/listen.addresses}}
     server_name {{_nginx_server_name}};
 
-    {{#listen.addresses}}
-      listen {{.}}:{{https_port}} ssl{{#default}} default_server so_keepalive=on{{/default}};
-    {{/listen.addresses}}
     {{#ssl_cert}}
     ssl_certificate {{ssl_cert}};
     ssl_certificate_key {{ssl_cert_key}};

test/proxy/test_nginx_rewrites.rb

@@ -9,6 +9,7 @@ def setup
   end
 
   def test_basic_rewrites
+    log_tail = LogTail.new("nginx/current")
     override_config({
       "hosts" => [
         {
@@ -24,6 +25,8 @@ def test_basic_rewrites
         },
       ],
     }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
       # Basic rewrite
       response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options)
       assert_response_code(301, response)
@@ -37,6 +40,7 @@ def test_basic_rewrites
   end
 
   def test_default_host
+    log_tail = LogTail.new("nginx/current")
     override_config({
       "hosts" => [
         {
@@ -51,6 +55,8 @@ def test_default_host
         },
       ],
     }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
       # Known host without rewrites
       response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
         :headers => { "Host" => "default.foo" },
@@ -74,6 +80,7 @@ def test_default_host
   end
 
   def test_no_default_host
+    log_tail = LogTail.new("nginx/current")
     override_config({
       "hosts" => [
         {
@@ -84,6 +91,8 @@ def test_no_default_host
         },
       ],
     }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
       # Known host without rewrites
       response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
         :headers => { "Host" => "default.foo" },
@@ -99,7 +108,138 @@ def test_no_default_host
     end
   end
 
+  # When nginx has no default_server defined, then the first server defined
+  # becomes the default one. So test to see how this ordering also interacts
+  # with a wildcard host.
+  def test_no_default_host_wildcard_first
+    log_tail = LogTail.new("nginx/current")
+    override_config({
+      "hosts" => [
+        {
+          "hostname" => "*",
+          "rewrites" => [
+            "^/#{unique_test_id}/hello/rewrite https://example.com/something/ permanent",
+          ],
+        },
+        {
+          "hostname" => "known.foo",
+        },
+      ],
+    }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
+      # Known host without rewrites
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "known.foo" },
+      }))
+      assert_response_code(404, response)
+
+      # Unknown host
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "unknown.foo" },
+      }))
+      assert_response_code(404, response)
+    end
+  end
+
+  def test_no_default_host_wildcard_last
+    log_tail = LogTail.new("nginx/current")
+    override_config({
+      "hosts" => [
+        {
+          "hostname" => "known.foo",
+        },
+        {
+          "hostname" => "*",
+          "rewrites" => [
+            "^/#{unique_test_id}/hello/rewrite https://example.com/something/ permanent",
+          ],
+        },
+      ],
+    }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
+      # Known host without rewrites
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "known.foo" },
+      }))
+      assert_response_code(404, response)
+
+      # Unknown host
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "unknown.foo" },
+      }))
+      assert_response_code(404, response)
+    end
+  end
+
+  def test_wildcard_is_default
+    log_tail = LogTail.new("nginx/current")
+    override_config({
+      "hosts" => [
+        {
+          "hostname" => "*",
+          "default" => true,
+          "rewrites" => [
+            "^/#{unique_test_id}/hello/rewrite https://example.com/something/ permanent",
+          ],
+        },
+        {
+          "hostname" => "known.foo",
+        },
+      ],
+    }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
+      # Known host without rewrites
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "known.foo" },
+      }))
+      assert_response_code(404, response)
+
+      # Unknown host
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "unknown.foo" },
+      }))
+      assert_response_code(301, response)
+      assert_equal("https://example.com/something/?foo=bar", response.headers["location"])
+    end
+  end
+
+  def test_wildcard_is_not_default
+    log_tail = LogTail.new("nginx/current")
+    override_config({
+      "hosts" => [
+        {
+          "hostname" => "*",
+          "rewrites" => [
+            "^/#{unique_test_id}/hello/rewrite https://example.com/something/ permanent",
+          ],
+        },
+        {
+          "hostname" => "known.foo",
+          "default" => true,
+        },
+      ],
+    }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
+      # Known host without rewrites
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "known.foo" },
+      }))
+      assert_response_code(404, response)
+
+      # Unknown host
+      response = Typhoeus.get("https://127.0.0.1:9081/#{unique_test_id}/hello/rewrite?foo=bar", http_options.deep_merge({
+        :headers => { "Host" => "unknown.foo" },
+      }))
+      assert_response_code(404, response)
+    end
+  end
+
   def test_precedence
+    log_tail = LogTail.new("nginx/current")
     override_config({
       "apis" => [
         {
@@ -129,6 +269,8 @@ def test_precedence
         },
       ],
     }, "--router") do
+      refute_nginx_duplicate_server_name_warnings(log_tail)
+
       http_opts = http_options.deep_merge({
         :headers => { "Host" => "with-apis-and-website.foo" },
       })
@@ -166,4 +308,13 @@ def test_precedence
       assert_match("<center>API Umbrella</center>", response.body)
     end
   end
+
+  private
+
+  def refute_nginx_duplicate_server_name_warnings(log_tail)
+    log_output = log_tail.read
+    assert_match(/\[notice\].*: reconfiguring/, log_output)
+    refute_match("conflicting server name", log_output)
+    refute_match("warn", log_output)
+  end
 end

test/support/log_tail.rb

@@ -0,0 +1,20 @@
+class LogTail
+  def initialize(filename)
+    @path = File.join($config["log_dir"], filename)
+    File.open(@path) do |file|
+      file.seek(0, IO::SEEK_END)
+      @pos = file.pos
+    end
+  end
+
+  def read
+    output = nil
+    File.open(@path) do |file|
+      file.seek(@pos)
+      output = file.read
+      @pos = file.pos
+    end
+
+    output
+  end
+end