Merge pull request #1500 from tkan145/THREESCALE-9301-dns-cache-miss

[THREESCALE-9301] Fix dns cache miss

Author: tkan145

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fixed Conditional policy evaluating incorrectly: second policy in policy chain that implement export() always triggers [PR #1485](https://github.com/3scale/APIcast/pull/1485) [THREESCALE-9320](https://issues.redhat.com/browse/THREESCALE-9320)
 - Fix APIcast using stale configuration for deleted products [PR #1488](https://github.com/3scale/APIcast/pull/1488) [THREESCALE-10130](https://issues.redhat.com/browse/THREESCALE-10130)
 - Fixed Mutual TLS between APIcast and the Backend API fails when using a Forward Proxy [PR #1499](https://github.com/3scale/APIcast/pull/1499) [THREESCALE-5105](https://issues.redhat.com/browse/THREESCALE-5105)
+- Fixed dns cache miss [PR #1500](https://github.com/3scale/APIcast/pull/1500) [THEESCALE-9301](https://issues.redhat.com/browse/THREESCALE-9301)
 
 ### Added
 

gateway/src/resty/resolver.lua

@@ -18,17 +18,18 @@ local dns_client = require 'resty.resolver.dns_client'
 local resty_env = require 'resty.env'
 local upstream = require 'ngx.upstream'
 local re = require('ngx.re')
-local semaphore = require "ngx.semaphore"
+local semaphore = require "ngx.semaphore".new(1)
 local synchronization = require('resty.synchronization').new(1)
-
-local init = semaphore.new(1)
+local table_new = require("table.new")
 
 local default_resolver_port = 53
 
 local _M = {
   _VERSION = '0.1',
 }
 
+local TYPE_A = 1
+
 local mt = { __index = _M }
 
 local function read_resolv_conf(path)
@@ -171,14 +172,14 @@ function _M.init_nameservers(path)
 end
 
 function _M.nameservers()
-  local ok, _ = init:wait(0)
+  local ok, _ = semaphore:wait(0)
 
   if ok and #(_M._nameservers) == 0 then
     _M.init()
   end
 
   if ok then
-    init:post()
+    semaphore:post()
   end
 
   return _M._nameservers
@@ -287,65 +288,68 @@ local function valid_answers(answers)
   return answers and not answers.errcode and #answers > 0 and (not answers.addresses or #answers.addresses > 0)
 end
 
-local function search_dns(self, qname, stale)
+local function resolve_upstream(qname)
+  local peers, err = upstream.get_primary_peers(qname)
 
-  local search = self.search
-  local dns = self.dns
-  local options = self.options
-  local cache = self.cache
+  if not peers then
+    return nil, err
+  end
 
-  local function get_answer(query)
-    local answers, err
-    answers, err = cache:get(query, stale)
-    if valid_answers(answers) then
-      return answers, err
-    end
+  for i=1, #peers do
+    local m = re.split(peers[i].name, ':', 'oj')
 
-    answers, err = dns:query(query, options)
-    if valid_answers(answers) then
-      cache:save(answers)
-      return answers, err
-    end
-    return nil, err
+    peers[i] = new_answer(m[1], m[2])
   end
 
+  return peers
+end
+
+-- construct search list from resolv options: search
+-- @param search table of search domain
+-- @param qname the name to query for
+-- @return table with search names
+local function search_list(search, qname)
+  -- FQDN
   if sub(qname, -1) == "." then
     local query = sub(qname, 1 ,-2)
-    ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' query: ', query)
-    return get_answer(query)
+    return {query}
   end
 
-  local answer, err
+  local names = table_new(#search +1, 0)
   for i=1, #search do
-    local query = qname .. '.' .. search[i]
-    ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' search: ', search[i], ' query: ', query)
-    answer, err = get_answer(query)
-    if answer then
-      return answer, err
-    end
+    names[i] = qname .. "." .. search[i]
   end
 
-  return nil, err
+  return names
 end
 
-local function resolve_upstream(qname)
-  local peers, err = upstream.get_primary_peers(qname)
+local function search_dns(self, qname)
 
-  if not peers then
-    return nil, err
-  end
+  local search = self.search
+  local dns = self.dns
+  local options = self.options
+  local queries = search_list(search, qname)
+  local answers, err
 
-  for i=1, #peers do
-    local m = re.split(peers[i].name, ':', 'oj')
+  -- Nothing found, append search domain and query DNS server
+  -- Return the first valid answer
+  for _, query in ipairs(queries) do
+    ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' query: ', query)
 
-    peers[i] = new_answer(m[1], m[2])
+    answers, err = dns:query(query, options)
+    if valid_answers(answers) then
+      return answers, err
+    end
   end
 
-  return peers
+  return nil, err
 end
 
+
 function _M.lookup(self, qname, stale)
   local cache = self.cache
+  local options = self.options
+  local qtype = options.qtype or TYPE_A
 
   ngx.log(ngx.DEBUG, 'resolver query: ', qname)
 
@@ -355,20 +359,28 @@ function _M.lookup(self, qname, stale)
     ngx.log(ngx.DEBUG, 'host is ip address: ', qname)
     answers = { new_answer(qname) }
   else
-    if is_fqdn(qname) then
-      answers, err = cache:get(qname, stale)
-    else
-      answers, err = resolve_upstream(qname)
+    local key = qname .. ":" .. qtype
+
+    -- Check cache first
+    answers, err = cache:get(key, stale)
+    if valid_answers(answers) then
+      return answers, nil
     end
 
-    if not valid_answers(answers) then
-      answers, err = search_dns(self, qname, stale)
+    if not is_fqdn(qname) then
+      answers, err = resolve_upstream(qname)
+
+      if valid_answers(answers) then
+        return answers, nil
+      end
     end
 
+    answers, err = search_dns(self, qname)
+    if answers then
+      cache:save(qname, qtype, answers)
+    end
   end
 
-  ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' finished with ', #(answers or empty), ' answers')
-
   return answers, err
 end
 
@@ -390,6 +402,7 @@ function _M.get_servers(self, qname, opts)
   local ok = sema:wait(0)
 
   local answers, err = self:lookup(qname, not ok)
+  ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' finished with ', #(answers or empty), ' answers')
 
   if ok then
     -- cleanup the key so we don't have unbounded growth of this table

gateway/src/resty/resolver/cache.lua

@@ -12,6 +12,7 @@ local _M = {
   _VERSION = '0.1'
 }
 
+
 local mt = { __index = _M }
 
 local shared_lrucache = resty_lrucache.new(1000)
@@ -35,17 +36,18 @@ local function compact_answers(servers)
 
     if server then
       local name = server.name or server.address
+      local type = server.type
 
       local packed = hash[name]
-
       if packed then
         insert(packed, server)
         packed.ttl = min(packed.ttl, server.ttl)
       else
         packed = {
           server,
           name = name,
-          ttl = server.ttl
+          ttl = server.ttl,
+          type = type,
         }
 
         insert(compact, packed)
@@ -57,7 +59,7 @@ local function compact_answers(servers)
   return compact
 end
 
-function _M.store(self, answer, force_ttl)
+function _M.store(self, qname, qtype, answer, force_ttl)
   local cache = self.cache
 
   if not cache then
@@ -71,23 +73,36 @@ function _M.store(self, answer, force_ttl)
     return nil, 'invalid answer'
   end
 
-  ngx.log(ngx.DEBUG, 'resolver cache write ', name, ' with TLL ', answer.ttl)
+  local type = answer.type
+
+  if not type then
+    ngx.log(ngx.WARN, 'resolver cache write refused invalid answer type ', inspect(answer))
+    return nil, 'invalid answer'
+  end
+
+  if type == qtype then
+    name = qname
+  end
+
 
   local ttl = force_ttl or answer.ttl
 
   if ttl == -1 then
     ttl = nil
   end
 
-  return cache:set(name, answer, ttl)
+  local key = name .. ":" .. qtype
+  ngx.log(ngx.DEBUG, 'resolver cache write ', key, ' with TLL ', ttl)
+
+  return cache:set(key, answer, ttl)
 end
 
 
-function _M.save(self, answers)
+function _M.save(self, qname, qtype, answers)
   local ans = compact_answers(answers or {})
 
   for _, answer in pairs(ans) do
-    local _, err = self:store(answer)
+    local _, err = self:store(qname, qtype, answer)
 
     if err then
       return nil, err

script/resolver

@@ -1,3 +1,3 @@
 #!/usr/bin/env sh
 
-exec resty -I apicast/src script/resolver.lua "$@"
+exec resty -I gateway/src script/resolver.lua "$@"

spec/resty/resolver/cache_spec.lua

@@ -39,7 +39,7 @@ describe('resty.resolver.cache', function()
     it('returns compacted answers', function()
       local keys = {}
 
-      for _,v in ipairs(c:save(answers)) do
+      for _,v in ipairs(c:save('www.example.com', 1, answers)) do
         table.insert(keys, v.name)
       end
 
@@ -51,7 +51,7 @@ describe('resty.resolver.cache', function()
     it('stores the result', function()
       c.store = spy.new(c.store)
 
-      c:save(answers)
+      c:save('eld.example.com', 1, answers)
 
       assert.spy(c.store).was.called(3) -- TODO: proper called_with(args)
     end)
@@ -63,40 +63,51 @@ describe('resty.resolver.cache', function()
 
     it('writes to the cache', function()
       local record = { 'someting' }
-      local answer = { record, ttl = 60, name = 'foo.example.com' }
+      local answer = { record, ttl = 60, name = 'foo.example.com', type = 1 }
       c.cache.set = spy.new(function(_, key, value, ttl)
-        assert.same('foo.example.com', key)
+        assert.same('foo.example.com:1', key)
         assert.same(answer, value)
         assert.same(60, ttl)
       end)
 
-      c:store(answer)
+      c:store('foo.example.com', 1, answer)
 
       assert.spy(c.cache.set).was.called(1)
     end)
 
     it('works with -1 ttl', function()
-      local answer = { { 'something' }, ttl = -1, name = 'foo.example.com' }
+      local answer = { { 'something' }, ttl = -1, name = 'foo.example.com', type = 1 }
 
       c.cache.set = spy.new(function(_, key, value, ttl)
-        assert.same('foo.example.com', key)
+        assert.same('foo.example.com:1', key)
         assert.same(answer, value)
         assert.same(nil, ttl)
       end)
 
-      c:store(answer)
+      c:store('foo.example.com', 1, answer)
 
       assert.spy(c.cache.set).was.called(1)
     end)
+
+    it('return error when name is missing', function()
+      local answer = { { 'something' }, ttl = -1 }
+      c.cache.set = spy.new(function(_, key, value, ttl)
+      end)
+
+      local _, err = c:store('something', 1, answer)
+
+      assert.same(err, "invalid answer")
+      assert.spy(c.cache.set).was_not_called()
+    end)
   end)
 
   describe('.get', function()
     local c = resolver_cache.new()
 
     it('returns answers', function()
-      c:save(answers)
+      c:save('www.example.com', 1, answers)
 
-      local ans = c:get('www.example.com')
+      local ans = c:get('www.example.com:1')
 
       assert.same({ "54.221.208.116", "54.221.221.16" }, ans.addresses)
     end)

spec/resty/resolver/http_spec.lua

@@ -14,7 +14,7 @@ describe('resty.resolver.http', function()
     it('resolves localhost', function()
       local client = _M.new()
       client:set_timeout(1000)
-      client.resolver.cache:save({ { address = '127.0.0.1', name = 'unknown.', ttl = 1800 } })
+      client.resolver.cache:save('unknown', 1, { { address = '127.0.0.1', name = 'unknown.', ttl = 1800 , type=1} })
       assert(client:connect({scheme="http", host='unknown', port=1984}))
       assert.equal('unknown', client.host)
       assert.equal(1984, client.port)

spec/resty/resolver/socket_spec.lua

@@ -16,7 +16,7 @@ describe('resty.resolver.socket', function()
       sock:settimeout(1000)
       local wrapper = _M.new(sock)
 
-      wrapper.resolver.cache:save({ { address = '127.0.0.1', name = 'unknown.', ttl = 1800 } })
+      wrapper.resolver.cache:save('unknown', 1, { { address = '127.0.0.1', name = 'unknown.', ttl = 1800, type = 1} })
       assert(wrapper:connect('unknown', 1984))
       assert.equal('unknown', wrapper.host)
       assert.equal(1984, wrapper.port)