[resolver] Refactor cache handling

Previously, we iterated through the answer and used the name contained in the
answer as the cache key. The problem with this is that if search domain is
added to the search query, the next query lookup will not hit the cache until
the same search domain is appened to the query.

With this PR, we use a combination of original qname and qtype as cache key
instead

gateway/src/resty/resolver.lua

@@ -28,6 +28,8 @@ local _M = {
   _VERSION = '0.1',
 }
 
+local TYPE_A = 1
+
 local mt = { __index = _M }
 
 local function read_resolv_conf(path)
@@ -321,25 +323,21 @@ local function search_list(search, qname)
   return names
 end
 
-local function search_dns(self, qname, stale)
+local function search_dns(self, qname)
 
   local search = self.search
   local dns = self.dns
   local options = self.options
-  local cache = self.cache
   local queries = search_list(search, qname)
   local answers, err
 
+  -- 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)
-    answers, err = cache:get(query, stale)
-    if valid_answers(answers) then
-      return answers, err
-    end
 
     answers, err = dns:query(query, options)
     if valid_answers(answers) then
-      cache:save(answers)
       return answers, err
     end
   end
@@ -350,6 +348,8 @@ 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)
 
@@ -359,19 +359,28 @@ function _M.lookup(self, qname, stale)
     ngx.log(ngx.DEBUG, 'host is ip address: ', qname)
     answers = { new_answer(qname) }
   else
-    answers, err = cache:get(qname, stale)
+    local key = qname .. ":" .. qtype
+
+    -- Check cache first
+    answers, err = cache:get(key, stale)
     if valid_answers(answers) then
       return answers, nil
     end
 
     if not is_fqdn(qname) then
       answers, err = resolve_upstream(qname)
+
+      if valid_answers(answers) then
+        return answers, nil
+      end
     end
 
-    if not valid_answers(answers) then
-      return search_dns(self, qname, stale)
+    answers, err = search_dns(self, qname)
+    if answers then
+      cache:save(qname, qtype, answers)
     end
   end
+
   return answers, err
 end
 
@@ -393,7 +402,6 @@ 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

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 ', name, ' 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

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)

spec/resty/resolver_spec.lua

@@ -55,8 +55,8 @@ describe('resty.resolver', function()
     it('skips answers with no address', function()
       dns.query = spy.new(function()
         return {
-          { name = 'www.3scale.net' , cname = '3scale.net' },
-          { name = '3scale.net' , address = '127.0.0.1' }
+          { name = 'www.3scale.net' , cname = '3scale.net', type = 5 },
+          { name = '3scale.net' , address = '127.0.0.1', type = 1 }
         }
       end)
 
@@ -151,6 +151,29 @@ describe('resty.resolver', function()
       assert.same(err, nil)
     end)
 
+    it("return cached value", function()
+      local dns = {
+        query = spy.new(function(_, name)
+            return { errcode = 3, errstr = 'name error' }
+        end)
+      }
+      local cache = resolver_cache.new()
+      local answer =  {{
+        address = "54.221.221.16",
+        class = 1,
+        name = "test.service.default.svc.cluster.local",
+        section = 1,
+        ttl = 59,
+        type = 1
+      }}
+
+      cache:save('test.service', 1, answer)
+      resolver = resty_resolver.new(dns, { cache = cache, search = {"default.svc.cluster.local"}})
+      local record, err = resolver:lookup('test.service')
+      assert.same("test.service.default.svc.cluster.local", record[1].name)
+      assert.same(err, nil)
+      assert.spy(dns.query).was.called(0)
+    end)
   end)
 
   describe('.parse_resolver', function()