From 48cd3a0270adf738af192243861af02a46ca194a Mon Sep 17 00:00:00 2001 From: Vinicius Mignot Date: Fri, 22 Mar 2024 11:33:14 -0300 Subject: [PATCH] feat(dns): function to force no sync toip() (#12739) --- .../unreleased/kong/force-no_sync-noip.yml | 5 ++ kong/resty/dns/client.lua | 38 ++++++++++---- spec/01-unit/21-dns-client/02-client_spec.lua | 50 +++++++++++++++++++ 3 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 changelog/unreleased/kong/force-no_sync-noip.yml diff --git a/changelog/unreleased/kong/force-no_sync-noip.yml b/changelog/unreleased/kong/force-no_sync-noip.yml new file mode 100644 index 000000000000..a4e9bac413cf --- /dev/null +++ b/changelog/unreleased/kong/force-no_sync-noip.yml @@ -0,0 +1,5 @@ +message: | + Added a new function to bypass the Kong's DNS client synchronization option + when resolving hostnames. +type: feature +scope: Core diff --git a/kong/resty/dns/client.lua b/kong/resty/dns/client.lua index 7725e5fb0f7a..2d929fae1e36 100644 --- a/kong/resty/dns/client.lua +++ b/kong/resty/dns/client.lua @@ -914,8 +914,10 @@ end -- @param dnsCacheOnly if true, no active lookup is done when there is no (stale) -- data. In that case an error is returned (as a dns server failure table). -- @param try_list the try_list object to add to +-- @param force_no_sync force noSyncronisation -- @return `entry + nil + try_list`, or `nil + err + try_list` -local function lookup(qname, r_opts, dnsCacheOnly, try_list) +local function lookup(qname, r_opts, dnsCacheOnly, try_list, force_no_sync) + local no_sync = noSynchronisation or force_no_sync or false local entry = cachelookup(qname, r_opts.qtype) if not entry then --not found in cache @@ -933,7 +935,7 @@ local function lookup(qname, r_opts, dnsCacheOnly, try_list) -- perform a sync lookup, as we have no stale data to fall back to try_list = try_add(try_list, qname, r_opts.qtype, "cache-miss") -- while kong is exiting, we cannot use timers and hence we run all our queries without synchronization - if noSynchronisation then + if no_sync then return individualQuery(qname, r_opts, try_list) elseif ngx.worker and ngx.worker.exiting() then log(DEBUG, PREFIX, "DNS query not synchronized because the worker is shutting down") @@ -1144,8 +1146,9 @@ end -- The field `additional_section` will default to `true` instead of `false`. -- @param dnsCacheOnly Only check the cache, won't do server lookups -- @param try_list (optional) list of tries to add to +-- @param force_no_sync force noSynchronisation -- @return `list of records + nil + try_list`, or `nil + err + try_list`. -local function resolve(qname, r_opts, dnsCacheOnly, try_list) +local function resolve(qname, r_opts, dnsCacheOnly, try_list, force_no_sync) qname = string_lower(qname) local qtype = (r_opts or EMPTY).qtype local err, records @@ -1187,7 +1190,7 @@ local function resolve(qname, r_opts, dnsCacheOnly, try_list) if (records[1] or EMPTY).type == _M.TYPE_CNAME and qtype ~= _M.TYPE_CNAME then opts.qtype = nil add_status_to_try_list(try_list, "dereferencing CNAME") - return resolve(records[1].cname, opts, dnsCacheOnly, try_list) + return resolve(records[1].cname, opts, dnsCacheOnly, try_list, force_no_sync) end -- return the shortname cache hit @@ -1230,7 +1233,7 @@ local function resolve(qname, r_opts, dnsCacheOnly, try_list) -- go look it up opts.qtype = try_type - records, err, try_list = lookup(try_name, opts, dnsCacheOnly, try_list) + records, err, try_list = lookup(try_name, opts, dnsCacheOnly, try_list, force_no_sync) if not records then -- An error has occurred, terminate the lookup process. We don't want to try other record types because -- that would potentially cause us to respond with wrong answers (i.e. the contents of an A record if the @@ -1287,7 +1290,7 @@ local function resolve(qname, r_opts, dnsCacheOnly, try_list) if records[1].type == _M.TYPE_CNAME and qtype ~= _M.TYPE_CNAME then opts.qtype = nil add_status_to_try_list(try_list, "dereferencing CNAME") - return resolve(records[1].cname, opts, dnsCacheOnly, try_list) + return resolve(records[1].cname, opts, dnsCacheOnly, try_list, force_no_sync) end return records, nil, try_list @@ -1487,17 +1490,18 @@ end -- representing the entire resolution history for a call. To prevent unnecessary -- string concatenations on a hot code path, it is not logged in this module. -- If you need to log it, just log `tostring(try_list)` from the caller code. --- @function toip +-- @function execute_toip -- @param qname hostname to resolve -- @param port (optional) default port number to return if none was found in -- the lookup chain (only SRV records carry port information, SRV with `port=0` will be ignored) -- @param dnsCacheOnly Only check the cache, won't do server lookups (will -- not invalidate any ttl expired data and will hence possibly return expired data) -- @param try_list (optional) list of tries to add to +-- @param force_no_sync force noSynchronisation = true for a single call -- @return `ip address + port + try_list`, or in case of an error `nil + error + try_list` -local function toip(qname, port, dnsCacheOnly, try_list) +local function execute_toip(qname, port, dnsCacheOnly, try_list, force_no_sync) local rec, err - rec, err, try_list = resolve(qname, nil, dnsCacheOnly, try_list) + rec, err, try_list = resolve(qname, nil, dnsCacheOnly, try_list, force_no_sync) if err then return nil, err, try_list end @@ -1507,13 +1511,26 @@ local function toip(qname, port, dnsCacheOnly, try_list) -- our SRV entry might still contain a hostname, so recurse, with found port number local srvport = (entry.port ~= 0 and entry.port) or port -- discard port if it is 0 add_status_to_try_list(try_list, "dereferencing SRV") - return toip(entry.target, srvport, dnsCacheOnly, try_list) + return execute_toip(entry.target, srvport, dnsCacheOnly, try_list) end -- must be A or AAAA return rec[roundRobin(rec)].address, port, try_list end +-- @see execute_toip +local function toip(...) + return execute_toip(...) +end + + +-- This function calls execute_toip() forcing it to always execute an individual +-- query for each resolve, ignoring the noSynchronisation option. +-- @see execute_toip +local function individual_toip(qname, port, dnsCacheOnly, try_list) + return execute_toip(qname, port, dnsCacheOnly, try_list, true) +end + --- Socket functions -- @section sockets @@ -1566,6 +1583,7 @@ end -- export local functions _M.resolve = resolve _M.toip = toip +_M.individual_toip = individual_toip _M.connect = connect _M.setpeername = setpeername diff --git a/spec/01-unit/21-dns-client/02-client_spec.lua b/spec/01-unit/21-dns-client/02-client_spec.lua index a4285089ed83..cb07c719deeb 100644 --- a/spec/01-unit/21-dns-client/02-client_spec.lua +++ b/spec/01-unit/21-dns-client/02-client_spec.lua @@ -1499,6 +1499,56 @@ describe("[DNS client]", function() assert.is_nil(ip) assert.are.equal("recursion detected", port) end) + + it("individual_noip - force no sync", function() + local resolve_count = 10 + assert(client.init({ + noSynchronisation = false, + })) + + local callcount = 0 + query_func = function(self, original_query_func, name, options) + callcount = callcount + 1 + return original_query_func(self, name, options) + end + + -- assert synchronisation is working + local threads = {} + for i=1,resolve_count do + threads[i] = ngx.thread.spawn(function() + local ip = client.toip("smtp." .. TEST_DOMAIN) + assert.is_string(ip) + end) + end + + -- only one thread must have called the query_func + assert.are.equal(1, callcount, + "synchronisation failed - out of " .. resolve_count .. " toip() calls " .. callcount .. + " queries were made") + + for i=1,#threads do + ngx.thread.wait(threads[i]) + end + + callcount = 0 + threads = {} + for i=1,resolve_count do + threads[i] = ngx.thread.spawn(function() + local ip = client.individual_toip("atest." .. TEST_DOMAIN) + assert.is_string(ip) + end) + end + + -- all threads must have called the query_func + assert.are.equal(resolve_count, callcount, + "force no sync failed - out of " .. resolve_count .. " toip() calls" .. + callcount .. " queries were made") + + for i=1,#threads do + ngx.thread.wait(threads[i]) + end + end) + end)