diff --git a/CHANGELOG.md b/CHANGELOG.md index f2385672a..c41588521 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/gateway/src/resty/resolver.lua b/gateway/src/resty/resolver.lua index 1b204b077..d2214a79d 100644 --- a/gateway/src/resty/resolver.lua +++ b/gateway/src/resty/resolver.lua @@ -18,10 +18,9 @@ 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 @@ -29,6 +28,8 @@ 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 diff --git a/gateway/src/resty/resolver/cache.lua b/gateway/src/resty/resolver/cache.lua index c77ee9806..aa9fba5e3 100644 --- a/gateway/src/resty/resolver/cache.lua +++ b/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,9 +36,9 @@ 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) @@ -45,7 +46,8 @@ local function compact_answers(servers) 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,7 +73,17 @@ 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 @@ -79,15 +91,18 @@ function _M.store(self, answer, force_ttl) 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 diff --git a/script/resolver b/script/resolver index 1d20f52ad..b52e32242 100755 --- a/script/resolver +++ b/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 "$@" diff --git a/spec/resty/resolver/cache_spec.lua b/spec/resty/resolver/cache_spec.lua index cbee15d01..964024e59 100644 --- a/spec/resty/resolver/cache_spec.lua +++ b/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) diff --git a/spec/resty/resolver/http_spec.lua b/spec/resty/resolver/http_spec.lua index fadd42330..bab71c252 100644 --- a/spec/resty/resolver/http_spec.lua +++ b/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) diff --git a/spec/resty/resolver/socket_spec.lua b/spec/resty/resolver/socket_spec.lua index f41b05df7..b647b8916 100644 --- a/spec/resty/resolver/socket_spec.lua +++ b/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) diff --git a/spec/resty/resolver_spec.lua b/spec/resty/resolver_spec.lua index e0e85893f..5e5c00651 100644 --- a/spec/resty/resolver_spec.lua +++ b/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()