fix(dns): ignore records with RR types differs from that of the query…

… when parsing answers (#13002)

Fix KAG-4446, FTI-5834

changelog/unreleased/kong/fix-dns-ignore-records-with-non-matching-types.yml

@@ -0,0 +1,3 @@
+message: "**DNS Client**: Ignore records with RR types differs from that of the query when parsing answers."
+type: bugfix
+scope: Core

kong/resty/dns/client.lua

@@ -45,7 +45,6 @@ local math_max = math.max
 local math_fmod = math.fmod
 local math_random = math.random
 local table_remove = table.remove
-local table_insert = table.insert
 local table_concat = table.concat
 local string_lower = string.lower
 local string_byte  = string.byte
@@ -666,15 +665,13 @@ _M.init = function(options)
 end
 
 
--- Removes non-requested results, updates the cache.
+-- Removes records with non-matching types, updates the cache.
 -- Parameter `answers` is updated in-place.
 -- @return `true`
 local function parseAnswer(qname, qtype, answers, try_list)
 
-  -- check the answers and store them in the cache
+  -- check the answers and store records with matching types in the cache
   -- eg. A, AAAA, SRV records may be accompanied by CNAME records
-  -- store them all, leaving only the requested type in so we can return that set
-  local others = {}
 
   -- remove last '.' from FQDNs as the answer does not contain it
   local check_qname do
@@ -691,25 +688,10 @@ local function parseAnswer(qname, qtype, answers, try_list)
     -- normalize casing
     answer.name = string_lower(answer.name)
 
-    if (answer.type ~= qtype) or (answer.name ~= check_qname) then
-      local key = answer.type..":"..answer.name
-      add_status_to_try_list(try_list, key .. " removed")
-      local lst = others[key]
-      if not lst then
-        lst = {}
-        others[key] = lst
-      end
-      table_insert(lst, 1, answer)  -- pos 1: preserve order
-      table_remove(answers, i)
-    end
-  end
-  if next(others) then
-    for _, lst in pairs(others) do
-      cacheinsert(lst)
-      -- set success-type, only if not set (this is only a 'by-product')
-      if not cachegetsuccess(lst[1].name) then
-        cachesetsuccess(lst[1].name, lst[1].type)
-      end
+    if answer.type ~= qtype then
+      table_remove(answers, i)  -- remove records with non-matching types
+    else
+      answer.name = check_qname
     end
   end
 

spec/01-unit/21-dns-client/02-client_spec.lua

@@ -784,29 +784,29 @@ describe("[DNS client]", function()
     -- check first CNAME
     local key1 = client.TYPE_CNAME..":"..host
     local entry1 = lrucache:get(key1)
-    assert.are.equal(host, entry1[1].name)       -- the 1st record is the original 'smtp.'..TEST_DOMAIN
-    assert.are.equal(client.TYPE_CNAME, entry1[1].type) -- and that is a CNAME
+    assert.falsy(entry1)
 
     -- check second CNAME
-    local key2 = client.TYPE_CNAME..":"..entry1[1].cname
+    local key2 = client.TYPE_CNAME..":thuis.kong-gateway-testing.link"
     local entry2 = lrucache:get(key2)
-    assert.are.equal(entry1[1].cname, entry2[1].name) -- the 2nd is the middle 'thuis.'..TEST_DOMAIN
-    assert.are.equal(client.TYPE_CNAME, entry2[1].type) -- and that is also a CNAME
+    assert.falsy(entry2)
 
-    -- check second target to match final record
-    assert.are.equal(entry2[1].cname, answers[1].name)
-    assert.are.not_equal(host, answers[1].name)  -- we got final name 'wdnaste.duckdns.org'
-    assert.are.equal(typ, answers[1].type)       -- we got a final A type record
+    assert.are.equal(host, answers[1].name) -- we got final name same to host
+    assert.are.equal(typ, answers[1].type)  -- we got a final A type record
     assert.are.equal(#answers, 1)
 
     -- check last successful lookup references
     local lastsuccess3 = lrucache:get(answers[1].name)
-    local lastsuccess2 = lrucache:get(entry2[1].name)
-    local lastsuccess1 = lrucache:get(entry1[1].name)
+    local lastsuccess2 = lrucache:get("thuis.kong-gateway-testing.link")
+    local lastsuccess1 = lrucache:get("kong-gateway-testing.link")
     assert.are.equal(client.TYPE_A, lastsuccess3)
-    assert.are.equal(client.TYPE_CNAME, lastsuccess2)
-    assert.are.equal(client.TYPE_CNAME, lastsuccess1)
+    assert.is_nil(lastsuccess2)
+    assert.is_nil(lastsuccess1)
 
+    -- check entries in the intermediate cache against the final output result
+    local key = client.TYPE_A .. ":" .. host
+    local entry = lrucache:get(key)
+    assert.same(answers, entry)
   end)
 
   it("fetching multiple SRV records (un-typed)", function()
@@ -839,17 +839,18 @@ describe("[DNS client]", function()
     -- first check CNAME
     local key = client.TYPE_CNAME..":"..host
     local entry = lrucache:get(key)
-    assert.are.equal(host, entry[1].name)
-    assert.are.equal(client.TYPE_CNAME, entry[1].type)
+    assert.falsy(entry)
 
     -- check final target
-    assert.are.equal(entry[1].cname, answers[1].name)
     assert.are.equal(typ, answers[1].type)
-    assert.are.equal(entry[1].cname, answers[2].name)
     assert.are.equal(typ, answers[2].type)
-    assert.are.equal(entry[1].cname, answers[3].name)
     assert.are.equal(typ, answers[3].type)
     assert.are.equal(#answers, 3)
+
+    -- check entries in the intermediate cache against the final output result
+    key = client.TYPE_SRV .. ":" .. host
+    entry = lrucache:get(key)
+    assert.same(answers, entry)
   end)
 
   it("fetching non-type-matching records", function()

spec/01-unit/21-dns-client/03-client_cache_spec.lua

@@ -402,13 +402,15 @@ describe("[DNS client cache]", function()
       -- in the cache, as in some cases lookups of certain types (eg. CNAME) are
       -- blocked, and then we rely on the A record to get them in the AS
       -- (additional section), but then they must be stored obviously.
-      local CNAME1 = {
+      local CNAME_from_A_query = {
         type = client.TYPE_CNAME,
         cname = "myotherhost.domain.com",
         class = 1,
         name = "myhost9.domain.com",
         ttl = 0.1,
       }
+      -- copy to make it different
+      local CNAME_from_CNAME_query = utils.cycle_aware_deep_copy(CNAME_from_A_query)
       local A2 = {
         type = client.TYPE_A,
         address = "1.2.3.4",
@@ -417,8 +419,8 @@ describe("[DNS client cache]", function()
         ttl = 60,
       }
       mock_records = setmetatable({
-        ["myhost9.domain.com:"..client.TYPE_CNAME] = { utils.cycle_aware_deep_copy(CNAME1) },  -- copy to make it different
-        ["myhost9.domain.com:"..client.TYPE_A] = { CNAME1, A2 },  -- not there, just a reference and target
+        ["myhost9.domain.com:"..client.TYPE_CNAME] = { CNAME_from_CNAME_query },
+        ["myhost9.domain.com:"..client.TYPE_A] = { CNAME_from_A_query, A2 },  -- not there, just a reference and target
         ["myotherhost.domain.com:"..client.TYPE_A] = { A2 },
       }, {
         -- do not do lookups, return empty on anything else
@@ -429,11 +431,115 @@ describe("[DNS client cache]", function()
       })
 
       assert(client.resolve("myhost9", { qtype = client.TYPE_CNAME }))
+      local cached = lrucache:get(client.TYPE_CNAME..":myhost9.domain.com")
+      assert.are.equal(CNAME_from_CNAME_query, cached[1])
+      assert.are.not_equal(CNAME_from_A_query, cached[1])
+
       ngx.sleep(0.2)  -- wait for it to become stale
+
       assert(client.toip("myhost9"))
 
+      -- CNAME entries are not stored into cache as `intermediates` for A type
+      -- queries, only A entries are stored.
+      cached = lrucache:get(client.TYPE_A..":myhost9.domain.com")
+      assert.are.equal(A2, cached[1])
+
+      -- The original cached CNAME entry will not be replaced by new CNAME entry.
+      cached = lrucache:get(client.TYPE_CNAME..":myhost9.domain.com")
+      assert.are.equal(CNAME_from_CNAME_query, cached[1])
+      assert.are.not_equal(CNAME_from_A_query, cached[1])
+    end)
+
+    it("No RRs match the queried type", function()
+      mock_records = {
+        ["myhost9.domain.com:"..client.TYPE_A] = {
+          {
+            type = client.TYPE_CNAME,
+            cname = "myotherhost.domain.com",
+            class = 1,
+            name = "myhost9.domain.com",
+            ttl = 60,
+          },
+        },
+      }
+
+      local res, _, try_list = client.resolve("myhost9", {})
+      assert.matches(tostring(try_list), '"myhost9.domain.com:1 - cache-miss/querying/dns client error: 101 empty record received"')
+      assert.is_nil(res)
+    end)
+  end)
+
+  describe("fqdn (simple dns_order)", function()
+
+    local lrucache, mock_records, config
+    before_each(function()
+      config = {
+        nameservers = { "198.51.100.0" },
+        ndots = 1,
+        hosts = {},
+        resolvConf = {},
+        order = { "LAST", "A" },
+        badTtl = 0.5,
+        staleTtl = 0.5,
+        enable_ipv6 = false,
+      }
+      assert(client.init(config))
+      lrucache = client.getcache()
+
+      query_func = function(self, original_query_func, qname, opts)
+        return mock_records[qname..":"..opts.qtype] or { errcode = 3, errstr = "name error" }
+      end
+    end)
+
+    -- FTI-5834
+    it("only query A type", function()
+      local CNAME = {
+        type = client.TYPE_CNAME,
+        cname = "myotherhost.domain.com",
+        class = 1,
+        name = "myhost9.domain.com",
+        ttl = 60,
+      }
+      local A = {
+        type = client.TYPE_A,
+        address = "1.2.3.4",
+        class = 1,
+        name = "myotherhost.domain.com",
+        ttl = 60,
+      }
+      mock_records = {
+        ["myhost9.domain.com:"..client.TYPE_A] = { CNAME, A }
+      }
+
+      local res = client.resolve("myhost9.domain.com")
+      -- The record's name has been fixed to the queried name.
+      assert.equal("myhost9.domain.com", res[1].name)
+      assert.equal(A, res[1])
+
+      local success = lrucache:get("myhost9.domain.com")
+      assert.equal(client.TYPE_A, success)
+
+      -- The CNAME records have been removed due to a mismatch with type A.
       local cached = lrucache:get(client.TYPE_CNAME..":myhost9.domain.com")
-      assert.are.equal(CNAME1, cached[1])
+      assert.is_nil(cached)
+    end)
+
+    it("No RRs match the queried type", function()
+      mock_records = {
+        ["myhost9.domain.com:"..client.TYPE_A] = {
+          {
+            type = client.TYPE_CNAME,
+            cname = "myotherhost.domain.com",
+            class = 1,
+            name = "myhost9.domain.com",
+            ttl = 60,
+          },
+        },
+      }
+
+      local res, err = client.resolve("myhost9.domain.com", {})
+      assert.equal('dns client error: 101 empty record received', err)
+      assert.is_nil(res)
     end)
 
   end)
@@ -519,9 +625,15 @@ describe("[DNS client cache]", function()
           },
         }
       }
+
       client.toip("demo.service.consul")
+
+      -- It only stores SRV target entry into cache, not storing A entry.
       local success = client.getcache():get("192.168.5.232.node.api_test.consul")
-      assert.equal(client.TYPE_A, success)
+      assert.is_nil(success)
+
+      success = client.getcache():get("demo.service.consul")
+      assert.equal(client.TYPE_SRV, success)
     end)
 
     it("are not overwritten by add. section info", function()