From ac0fd3c5aade820c68e7190311ea35b6fe72b368 Mon Sep 17 00:00:00 2001 From: An Tran Date: Mon, 14 Oct 2024 15:01:23 +1000 Subject: [PATCH] [resolver] refactor search list Now return the full list of names to search for. This allows us to iterate over the list and check the cache entry before querying the DNS server. --- CHANGELOG.md | 1 + gateway/src/resty/resolver.lua | 72 +++++++++++++++++++++++----------- spec/resty/resolver_spec.lua | 51 ++++++++++++++++++++++-- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5356f746..fbe9d73d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,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 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 38a73886c..8733f8149 100644 --- a/gateway/src/resty/resolver.lua +++ b/gateway/src/resty/resolver.lua @@ -1,3 +1,12 @@ +-- DNS Resolver +-- +-- _NOTES_: +-- +-- 1. Parsing the config file (/etc/resolv.conf) use blocking I/O, so it should +-- be called in init phase. See `init` for details +-- 2. TTL for records is the TTL returned by DNS server and won't be updated +-- while the client serves the records from its cache + local setmetatable = setmetatable local next = next local open = io.open @@ -9,15 +18,17 @@ local find = string.find local insert = table.insert local concat = table.concat local io_type = io.type +local table_new = require("table.new") require('resty.core.regex') -- to allow use of ngx.re.match in the init phase +local ngx_re = require('ngx.re') +local re_split = ngx_re.split local re_match = ngx.re.match local resolver_cache = require 'resty.resolver.cache' 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".new(1) local synchronization = require('resty.synchronization').new(1) @@ -126,7 +137,7 @@ function _M.parse_nameservers(path) insert(nameservers, resolver) end - for _,line in ipairs(re.split(resolv_conf, "\n+")) do + for _,line in ipairs(re_split(resolv_conf, "\n+")) do local domains = match(line, '^search%s+([^\n]+)') @@ -182,6 +193,8 @@ function _M.nameservers() return _M._nameservers end +--- Initialize the resolver. +-- @param path to resolve.conf file function _M.init(path) _M.init_nameservers(path) end @@ -285,42 +298,50 @@ local function valid_answers(answers) return answers and not answers.errcode and #answers > 0 and (not answers.addresses or #answers.addresses > 0) 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 + return {qname} + end + + local names = table_new(#search +1, 0) + for i=1, #search do + names[i] = qname .. "." .. search[i] + end + return names +end + local function search_dns(self, qname, stale) local search = self.search local dns = self.dns local options = self.options local cache = self.cache + local answers, err + local names = search_list(search, qname) - local function get_answer(query) - local answers, err + -- Search the cache first + for _, query in ipairs(names) do answers, err = cache:get(query, stale) if valid_answers(answers) then return answers, err end + end + + -- Nothing found, send DNS queries to resolve all `names` and return the + -- first valid answers + for _, query in ipairs(names) do + ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' query: ', query) answers, err = dns:query(query, options) if valid_answers(answers) then cache:save(answers) return answers, err end - return nil, err - end - - if sub(qname, -1) == "." then - local query = sub(qname, 1 ,-2) - ngx.log(ngx.DEBUG, 'resolver query: ', qname, ' query: ', query) - return get_answer(query) - end - - local answer, err - 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 end return nil, err @@ -334,7 +355,7 @@ local function resolve_upstream(qname) end for i=1, #peers do - local m = re.split(peers[i].name, ':', 'oj') + local m = re_split(peers[i].name, ':', 'oj') peers[i] = new_answer(m[1], m[2]) end @@ -342,6 +363,13 @@ local function resolve_upstream(qname) return peers end +-- Queries the DNS for this qname. If nothing is in the cache, a new DNS query +-- will be sent. If stale data is available in the cache, first query is going +-- to refresh the query while all others use stale cache data. +-- +-- @param qname the name to look for +-- @param stale if true return stale data +-- @return dns `answers + nil`, or `nil + error function _M.lookup(self, qname, stale) local cache = self.cache diff --git a/spec/resty/resolver_spec.lua b/spec/resty/resolver_spec.lua index e0e85893f..fdedcd40e 100644 --- a/spec/resty/resolver_spec.lua +++ b/spec/resty/resolver_spec.lua @@ -1,5 +1,6 @@ local resty_resolver = require 'resty.resolver' local resolver_cache = require 'resty.resolver.cache' +local string_byte = string.byte describe('resty.resolver', function() @@ -127,13 +128,23 @@ describe('resty.resolver', function() describe(':lookup', function() local resolver + local dns before_each(function() - local dns = { + dns = { query = spy.new(function(_, name) - return { - { name = name , address = '127.0.0.1' } - } + -- FQDN, drop the last '.' as the answer does not contain it + + if string_byte(name, -1) == string_byte(".") then + name = name:sub(1, -2) + end + if name == '3scale' then + return { errcode = 3, errstr = 'name error' } + else + return { + { name = name , address = '127.0.0.1' } + } + end end) } resolver = resty_resolver.new(dns, { cache = resolver_cache.new(), search = {"foo.com"}}) @@ -151,6 +162,38 @@ describe('resty.resolver', function() assert.same(err, nil) end) + it("search entry in cache before sending new request", function() + local dns = { + query = spy.new(function(_, name) + -- FQDN, drop the last '.' as the answer does not contain it + + if string_byte(name, -1) == string_byte(".") then + name = name:sub(1, -2) + end + if name == '3scale' then + return { errcode = 3, errstr = 'name error' } + else + return { + { name = name , address = '127.0.0.1' } + } + end + end) + } + resolver = resty_resolver.new(dns, { cache = resolver_cache.new(), search = {"foo.com"}}) + -- First populate the cache with new entry + local answer, err = resolver:lookup('3scale.foo.com.') + assert.same("3scale.foo.com", answer[1].name) + assert.same(err, nil) + + -- Now search for shortname, it should append the name with search + -- domain and lookup in cache first. + local answer, err = resolver:lookup('3scale') + assert.same("3scale.foo.com", answer[1].name) + assert.same(err, nil) + + -- Should only called 1 time + assert.spy(dns.query).was_called(1) + end) end) describe('.parse_resolver', function()