Merge pull request #4422 from ElvinEfendi/lua-resolv-conf-search

teach lua about search and ndots settings in resolv.conf

rootfs/etc/nginx/lua/balancer.lua

@@ -1,7 +1,7 @@
 local ngx_balancer = require("ngx.balancer")
 local cjson = require("cjson.safe")
 local util = require("util")
-local dns_util = require("util.dns")
+local dns_lookup = require("util.dns").lookup
 local configuration = require("configuration")
 local round_robin = require("balancer.round_robin")
 local chash = require("balancer.chash")
@@ -52,7 +52,7 @@ local function resolve_external_names(original_backend)
   local backend = util.deepcopy(original_backend)
   local endpoints = {}
   for _, endpoint in ipairs(backend.endpoints) do
-    local ips = dns_util.resolve(endpoint.address)
+    local ips = dns_lookup(endpoint.address)
     for _, ip in ipairs(ips) do
       table.insert(endpoints, { address = ip, port = endpoint.port })
     end

rootfs/etc/nginx/lua/tcp_udp_balancer.lua

@@ -1,7 +1,7 @@
 local ngx_balancer = require("ngx.balancer")
 local cjson = require("cjson.safe")
 local util = require("util")
-local dns_util = require("util.dns")
+local dns_lookup = require("util.dns").lookup
 local configuration = require("tcp_udp_configuration")
 local round_robin = require("balancer.round_robin")
 
@@ -34,7 +34,7 @@ local function resolve_external_names(original_backend)
   local backend = util.deepcopy(original_backend)
   local endpoints = {}
   for _, endpoint in ipairs(backend.endpoints) do
-    local ips = dns_util.resolve(endpoint.address)
+    local ips = dns_lookup(endpoint.address)
     for _, ip in ipairs(ips) do
       table.insert(endpoints, {address = ip, port = endpoint.port})
     end

rootfs/etc/nginx/lua/test/balancer_test.lua

@@ -315,7 +315,7 @@ describe("Balancer", function()
         }
       }
 
-      helpers.mock_resty_dns_query({
+      helpers.mock_resty_dns_query(nil, {
         {
           name = "example.com",
           address = "192.168.1.1",

rootfs/etc/nginx/lua/test/helpers.lua

@@ -34,10 +34,13 @@ function _M.mock_resty_dns_new(func)
   resty_dns_resolver.new = func
 end
 
-function _M.mock_resty_dns_query(response, err)
+function _M.mock_resty_dns_query(mocked_host, response, err)
   resty_dns_resolver.new = function(self, options)
     local r = original_resty_dns_resolver_new(self, options)
-    r.query = function(self, name, options, tries)
+    r.query = function(self, host, options, tries)
+      if mocked_host and mocked_host ~= host then
+        return error(tostring(host) .. " is not mocked")
+      end
       return response, err
     end
     return r

rootfs/etc/nginx/lua/test/run.lua

@@ -35,7 +35,7 @@ do
 end
 
 _G.helpers = require("test.helpers")
-
+_G._TEST = true
 
 local ffi = require("ffi")
 local lua_ingress = require("lua_ingress")

rootfs/etc/nginx/lua/test/util/dns_test.lua

@@ -9,45 +9,81 @@ helpers.with_resolv_conf(conf, function()
   require("util.resolv_conf")
 end)
 
-describe("resolve", function()
-  local dns = require("util.dns")
+describe("dns.lookup", function()
+  local dns, dns_lookup, spy_ngx_log
+
+  before_each(function()
+    spy_ngx_log = spy.on(ngx, "log")
+    dns = require("util.dns")
+    dns_lookup = dns.lookup
+  end)
+
+  after_each(function()
+    package.loaded["util.dns"] = nil
+  end)
 
   it("sets correct nameservers", function()
     helpers.mock_resty_dns_new(function(self, options)
       assert.are.same({ nameservers = { "1.2.3.4", "4.5.6.7" }, retrans = 5, timeout = 2000 }, options)
       return nil, ""
     end)
-    dns.resolve("example.com")
+    dns_lookup("example.com")
   end)
 
-  it("returns host when an error happens", function()
-    local s_ngx_log = spy.on(ngx, "log")
-
+  describe("when there's an error", function()
+    it("returns host when resolver can not be instantiated", function()
       helpers.mock_resty_dns_new(function(...) return nil, "an error" end)
-    assert.are.same({ "example.com" }, dns.resolve("example.com"))
-    assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to instantiate the resolver: an error")
-
-    helpers.mock_resty_dns_query(nil, "oops!")
-    assert.are.same({ "example.com" }, dns.resolve("example.com"))    
-    assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\noops!\noops!")
-
-    helpers.mock_resty_dns_query({ errcode = 1, errstr = "format error" })
-    assert.are.same({ "example.com" }, dns.resolve("example.com"))
-    assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nserver returned error code: 1: format error\nserver returned error code: 1: format error")
-
-    helpers.mock_resty_dns_query({})
-    assert.are.same({ "example.com" }, dns.resolve("example.com"))
-    assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nno record resolved\nno record resolved")
-
-    helpers.mock_resty_dns_query({ { name = "example.com", cname = "sub.example.com", ttl = 60 } })
-    assert.are.same({ "example.com" }, dns.resolve("example.com"))
-    assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nno record resolved\nno record resolved")
+      assert.are.same({ "example.com" }, dns_lookup("example.com"))
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to instantiate the resolver: an error")
     end)
 
-  it("resolves all A records of given host, caches them with minimal ttl and returns from cache next time", function()
-    helpers.mock_resty_dns_query({
+    it("returns host when the query returns nil", function()
+      helpers.mock_resty_dns_query(nil, nil, "oops!")
+      assert.are.same({ "example.com" }, dns_lookup("example.com"))    
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\noops!\noops!")
+    end)
+
+    it("returns host when the query returns empty answer", function()
+      helpers.mock_resty_dns_query(nil, {})
+      assert.are.same({ "example.com" }, dns_lookup("example.com"))
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nno A record resolved\nno AAAA record resolved")
+    end)
+
+    it("returns host when there's answer but with error", function()
+      helpers.mock_resty_dns_query(nil, { errcode = 1, errstr = "format error" })
+      assert.are.same({ "example.com" }, dns_lookup("example.com"))
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\n" ..
+        "server returned error code: 1: format error\nserver returned error code: 1: format error")
+    end)
+
+    it("retuns host when there's answer but no A/AAAA record in it", function()
+      helpers.mock_resty_dns_query(nil, { { name = "example.com", cname = "sub.example.com", ttl = 60 } })
+      assert.are.same({ "example.com" }, dns_lookup("example.com"))
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nno A record resolved\nno AAAA record resolved")
+    end)
+
+    it("returns host when the query returns nil and number of dots is not less than configured ndots", function()
+      helpers.mock_resty_dns_query(nil, nil, "oops!")
+      assert.are.same({ "a.b.c.d.example.com" }, dns_lookup("a.b.c.d.example.com"))    
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\noops!\noops!")
+    end)
+
+    it("returns host when the query returns nil for a fully qualified domain", function()
+      helpers.mock_resty_dns_query("example.com.", nil, "oops!")
+      assert.are.same({ "example.com." }, dns_lookup("example.com."))
+      assert.spy(spy_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\noops!\noops!")
+    end)
+  end)
+
+  it("returns answer from cache if it exists without doing actual DNS query", function()
+    dns._cache:set("example.com", { "192.168.1.1" })
+    assert.are.same({ "192.168.1.1" }, dns_lookup("example.com"))
+  end)
+
+  it("resolves a fully qualified domain without looking at resolv.conf search and caches result", function()
+    helpers.mock_resty_dns_query("example.com.", {
       {
-        name = "example.com",
+        name = "example.com.",
         address = "192.168.1.1",
         ttl = 3600,
       },
@@ -57,28 +93,43 @@ describe("resolve", function()
         ttl = 60,
       }
     })
-
-    local lrucache = require("resty.lrucache")
-    local old_lrucache_new = lrucache.new
-    lrucache.new = function(...)
-      local cache = old_lrucache_new(...)
-
-      local old_set = cache.set
-      cache.set = function(self, key, value, ttl)
-        assert.equal("example.com", key)
-        assert.are.same({ "192.168.1.1", "1.2.3.4" }, value)
-        assert.equal(60, ttl)
-        return old_set(self, key, value, ttl)
-      end
-
-      return cache
-    end
-
-    assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns.resolve("example.com"))
-
-    helpers.mock_resty_dns_new(function(...)
-      error("expected to short-circuit and return response from cache")
+    assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns_lookup("example.com."))
+    assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns._cache:get("example.com."))
   end)
-    assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns.resolve("example.com"))
+
+  it("starts with host itself when number of dots is not less than configured ndots", function()
+    local host = "a.b.c.d.example.com"
+    helpers.mock_resty_dns_query(host, { { name = host, address = "192.168.1.1", ttl = 3600, } } )
+
+    assert.are.same({ "192.168.1.1" }, dns_lookup(host))
+    assert.are.same({ "192.168.1.1" }, dns._cache:get(host))
+  end)
+
+  it("starts with first search entry when number of dots is less than configured ndots", function()
+    local host = "example.com.ingress-nginx.svc.cluster.local"
+    helpers.mock_resty_dns_query(host, { { name = host, address = "192.168.1.1", ttl = 3600, } } )
+
+    assert.are.same({ "192.168.1.1" }, dns_lookup(host))
+    assert.are.same({ "192.168.1.1" }, dns._cache:get(host))
+  end)
+
+  it("it caches with minimal ttl", function()
+    helpers.mock_resty_dns_query("example.com.", {
+      {
+        name = "example.com.",
+        address = "192.168.1.1",
+        ttl = 3600,
+      },
+      {
+        name = "example.com.",
+        address = "1.2.3.4",
+        ttl = 60,
+      }
+    })
+
+    local spy_cache_set = spy.on(dns._cache, "set")
+
+    assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns_lookup("example.com."))
+    assert.spy(spy_cache_set).was_called_with(match.is_table(), "example.com.", { "192.168.1.1", "1.2.3.4" }, 60)
   end)
 end)

rootfs/etc/nginx/lua/util/dns.lua

@@ -2,23 +2,48 @@ local resolver = require("resty.dns.resolver")
 local lrucache = require("resty.lrucache")
 local resolv_conf = require("util.resolv_conf")
 
+local ngx_log = ngx.log
+local ngx_INFO = ngx.INFO
+local ngx_ERR = ngx.ERR
+local string_format = string.format
+local table_concat = table.concat
+local table_insert = table.insert
+local ipairs = ipairs
+local tostring = tostring
+
 local _M = {}
 local CACHE_SIZE = 10000
 local MAXIMUM_TTL_VALUE = 2147483647 -- maximum value according to https://tools.ietf.org/html/rfc2181
+-- for every host we will try two queries for the following types with the order set here
+local QTYPES_TO_CHECK = { resolver.TYPE_A, resolver.TYPE_AAAA }
 
-local cache, err = lrucache.new(CACHE_SIZE)
-if not cache then
+local cache
+do
+  local err
+  cache, err = lrucache.new(CACHE_SIZE)
+  if not cache then
     return error("failed to create the cache: " .. (err or "unknown"))
+  end
 end
 
-local function a_records_and_max_ttl(answers)
+local function cache_set(host, addresses, ttl)
+  cache:set(host, addresses, ttl)
+  ngx_log(ngx_INFO, string_format("cache set for '%s' with value of [%s] and ttl of %s.",
+    host, table_concat(addresses, ", "), ttl))
+end
+
+local function is_fully_qualified(host)
+  return host:sub(-1) == "."
+end
+
+local function a_records_and_min_ttl(answers)
   local addresses = {}
   local ttl = MAXIMUM_TTL_VALUE -- maximum value according to https://tools.ietf.org/html/rfc2181
 
   for _, ans in ipairs(answers) do
     if ans.address then
-      table.insert(addresses, ans.address)
-      if ttl > ans.ttl then
+      table_insert(addresses, ans.address)
+      if ans.ttl < ttl then
         ttl = ans.ttl
       end
     end
@@ -27,71 +52,109 @@ local function a_records_and_max_ttl(answers)
   return addresses, ttl
 end
 
-local function resolve_host(host, r, qtype)
-  local answers
-  answers, err = r:query(host, { qtype = qtype }, {})
+local function resolve_host_for_qtype(r, host, qtype)
+  ngx_log(ngx_INFO, string_format("resolving %s with qtype %s", host, qtype))
+
+  local answers, err = r:query(host, { qtype = qtype }, {})
   if not answers then
-    return nil, -1, tostring(err)
+    return nil, -1, err
   end
 
   if answers.errcode then
-    return nil, -1, string.format("server returned error code: %s: %s", answers.errcode, answers.errstr)
+    return nil, -1, string_format("server returned error code: %s: %s", answers.errcode, answers.errstr)
   end
 
-  local addresses, ttl = a_records_and_max_ttl(answers)
+  local addresses, ttl = a_records_and_min_ttl(answers)
   if #addresses == 0 then
-    return nil, -1, "no record resolved"
+    local msg = "no A record resolved"
+    if qtype == resolver.TYPE_AAAA then msg = "no AAAA record resolved" end
+    return nil, -1, msg
   end
 
   return addresses, ttl, nil
 end
 
-function _M.resolve(host)
+local function resolve_host(r, host)
+  local dns_errors = {}
+
+  for _, qtype in ipairs(QTYPES_TO_CHECK) do
+    local addresses, ttl, err = resolve_host_for_qtype(r, host, qtype)
+    if addresses and #addresses > 0 then
+      return addresses, ttl, nil
+    end
+    table_insert(dns_errors, tostring(err))
+  end
+
+  return nil, nil, dns_errors
+end
+
+function _M.lookup(host)
   local cached_addresses = cache:get(host)
   if cached_addresses then
-    local message = string.format(
-      "addresses %s for host %s was resolved from cache",
-      table.concat(cached_addresses, ", "), host)
-    ngx.log(ngx.INFO, message)
     return cached_addresses
   end
 
-  local r
-  r, err = resolver:new{
+  local r, err = resolver:new{
     nameservers = resolv_conf.nameservers,
     retrans = 5,
     timeout = 2000,  -- 2 sec
   }
 
   if not r then
-    ngx.log(ngx.ERR, "failed to instantiate the resolver: " .. tostring(err))
+    ngx_log(ngx_ERR, string_format("failed to instantiate the resolver: %s", err))
     return { host }
   end
 
-  local dns_errors = {}
+  local addresses, ttl, dns_errors
 
-  local addresses, ttl
-  addresses, ttl, err = resolve_host(host, r, r.TYPE_A)
-  if not addresses then
-    table.insert(dns_errors, tostring(err))
-  elseif #addresses > 0 then
-    cache:set(host, addresses, ttl)
+  -- when the queried domain is fully qualified
+  -- then we don't go through resolv_conf.search
+  -- NOTE(elvinefendi): currently FQDN as externalName will be supported starting
+  -- with K8s 1.15: https://github.com/kubernetes/kubernetes/pull/78385
+  if is_fully_qualified(host) then
+    addresses, ttl, dns_errors = resolve_host(r, host)
+    if addresses then
+      cache_set(host, addresses, ttl)
       return addresses
     end
 
-  addresses, ttl, err = resolve_host(host, r, r.TYPE_AAAA)
-  if not addresses then
-    table.insert(dns_errors, tostring(err))
-  elseif #addresses > 0 then
-    cache:set(host, addresses, ttl)
+    ngx_log(ngx_ERR, "failed to query the DNS server:\n" .. table_concat(dns_errors, "\n"))
+
+    return { host }
+  end
+
+  -- for non fully qualified domains if number of dots in
+  -- the queried host is less than resolv_conf.ndots then we try
+  -- with all the entries in resolv_conf.search before trying the original host
+  --
+  -- if number of dots is not less than resolv_conf.ndots then we start with
+  -- the original host and then try entries in resolv_conf.search
+  local _, host_ndots = host:gsub("%.", "")
+  local search_start, search_end = 0, #resolv_conf.search
+  if host_ndots < resolv_conf.ndots then
+    search_start = 1
+    search_end = #resolv_conf.search + 1
+  end
+
+  for i = search_start, search_end, 1 do
+    local new_host = resolv_conf.search[i] and string_format("%s.%s", host, resolv_conf.search[i]) or host
+
+    addresses, ttl, dns_errors = resolve_host(r, new_host)
+    if addresses then
+      cache_set(host, addresses, ttl)
       return addresses
     end
+  end
 
   if #dns_errors > 0 then
-    ngx.log(ngx.ERR, "failed to query the DNS server:\n" .. table.concat(dns_errors, "\n"))
+    ngx_log(ngx_ERR, "failed to query the DNS server:\n" .. table_concat(dns_errors, "\n"))
   end
 
   return { host }
 end
 
+if _TEST then
+  _M._cache = cache
+end
+
 return _M

test/e2e/servicebackend/service_externalname.go

@@ -41,6 +41,40 @@ var _ = framework.IngressNginxDescribe("Service Type ExternalName", func() {
 	AfterEach(func() {
 	})
 
+	It("works with external name set to incomplete fdqn", func() {
+		f.NewEchoDeployment()
+
+		host := "echo"
+
+		svc := &core.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "httpbin",
+				Namespace: f.Namespace,
+			},
+			Spec: corev1.ServiceSpec{
+				ExternalName: "http-svc",
+				Type:         corev1.ServiceTypeExternalName,
+			},
+		}
+
+		f.EnsureService(svc)
+
+		ing := framework.NewSingleIngress(host, "/", host, f.Namespace, "httpbin", 80, nil)
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, "proxy_pass http://upstream_balancer;")
+			})
+
+		resp, _, errs := gorequest.New().
+			Get(f.GetURL(framework.HTTP)+"/get").
+			Set("Host", host).
+			End()
+		Expect(errs).Should(BeEmpty())
+		Expect(resp.StatusCode).Should(Equal(200))
+	})
+
 	It("should return 200 for service type=ExternalName without a port defined", func() {
 		host := "echo"