Skip to content

Commit

Permalink
tests(dns): flakiness due to hook conflict (#13298)
Browse files Browse the repository at this point in the history 
The test suits hook the query function and uses a shared variable to change the hook. This causes conflicts when test A has an aysnc query still running, and B swaps the hook function, so the function will be called more than expected times.

KAG-4520
  • Loading branch information
@StarlightIbuki
StarlightIbuki authored Jul 1, 2024
1 parent 63c01e5 commit 4bd17e1
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 30 deletions.
42 changes: 23 additions & 19 deletions spec/01-unit/21-dns-client/02-client_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,32 @@ end

describe("[DNS client]", function()

local client, resolver, query_func
local client, resolver

before_each(function()
client = require("kong.resty.dns.client")
resolver = require("resty.dns.resolver")

-- you can replace this `query_func` upvalue to spy on resolver query calls.
-- `resolver.query_func` is hooked to inspect resolver query calls. New values can be assigned to it.
-- This default will just call the original resolver (hence is transparent)
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
return original_query_func(self, name, options)
end

-- patch the resolver lib, such that any new resolver created will query
-- using the `query_func` upvalue defined above
-- using the `resolver.query_func` defined above
local old_new = resolver.new
resolver.new = function(...)
local r, err = old_new(...)
if not r then
return nil, err
end
local original_query_func = r.query

-- remember the passed in query_func
-- so it won't be replaced by the next resolver.new call
-- and won't interfere with other tests
local query_func = resolver.query_func
r.query = function(self, ...)
return query_func(self, original_query_func, ...)
end
Expand All @@ -70,7 +75,6 @@ describe("[DNS client]", function()
package.loaded["resty.dns.resolver"] = nil
client = nil
resolver = nil
query_func = nil
end)

describe("initialization", function()
Expand Down Expand Up @@ -583,7 +587,7 @@ describe("[DNS client]", function()
"127.0.0.1 host"
}
}))
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
-- The first request uses syncQuery not waiting on the
-- aysncQuery timer, so the low-level r:query() could not sleep(5s),
-- it can only sleep(timeout).
Expand Down Expand Up @@ -613,7 +617,7 @@ describe("[DNS client]", function()
-- KAG-2300 - https://github.com/Kong/kong/issues/10182
-- If we encounter a timeout while talking to the DNS server, don't keep trying with other record types
assert(client.init({ timeout = 1000, retrans = 2 }))
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
if options.qtype == client.TYPE_SRV then
ngx.sleep(10)
else
Expand Down Expand Up @@ -713,7 +717,7 @@ describe("[DNS client]", function()
it("fetching names case insensitive", function()
assert(client.init())

query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
return {
{
name = "some.UPPER.case",
Expand Down Expand Up @@ -901,7 +905,7 @@ describe("[DNS client]", function()
assert(client.init())

local callcount = 0
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
callcount = callcount + 1
return original_query_func(self, name, options)
end
Expand Down Expand Up @@ -949,7 +953,7 @@ describe("[DNS client]", function()
assert(client.init())

local callcount = 0
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
callcount = callcount + 1
return original_query_func(self, name, options)
end
Expand Down Expand Up @@ -996,7 +1000,7 @@ describe("[DNS client]", function()
},
}

query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
if name == host and options.qtype == client.TYPE_SRV then
return entry
end
Expand All @@ -1019,7 +1023,7 @@ describe("[DNS client]", function()
"nameserver 198.51.100.0",
},
}))
query_func = function(self, original_query_func, name, opts)
resolver.query_func = function(self, original_query_func, name, opts)
if name ~= "hello.world" and (opts or {}).qtype ~= client.TYPE_CNAME then
return original_query_func(self, name, opts)
end
Expand Down Expand Up @@ -1509,7 +1513,7 @@ describe("[DNS client]", function()
}))

local callcount = 0
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
callcount = callcount + 1
-- Introducing a simulated network delay ensures individual_toip always
-- triggers a DNS query to avoid it triggering only once due to a cache
Expand Down Expand Up @@ -1575,7 +1579,7 @@ describe("[DNS client]", function()
}))

-- mock query function to return a default record
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
return {
{
type = client.TYPE_A,
Expand Down Expand Up @@ -1613,7 +1617,7 @@ describe("[DNS client]", function()

-- mock query function to count calls
local call_count = 0
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
call_count = call_count + 1
return original_query_func(self, name, options)
end
Expand Down Expand Up @@ -1688,7 +1692,7 @@ describe("[DNS client]", function()

-- mock query function to count calls, and return errors
local call_count = 0
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
call_count = call_count + 1
return { errcode = 5, errstr = "refused" }
end
Expand Down Expand Up @@ -1754,7 +1758,7 @@ describe("[DNS client]", function()
local results = {}

local call_count = 0
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
call_count = call_count + 1
sleep(0.5) -- make sure we take enough time so the other threads
-- will be waiting behind this one
Expand Down Expand Up @@ -1817,7 +1821,7 @@ describe("[DNS client]", function()

-- insert a stub thats waits and returns a fixed record
local name = TEST_DOMAIN
query_func = function()
resolver.query_func = function()
local ip = ip
local entry = {
{
Expand Down Expand Up @@ -1884,7 +1888,7 @@ describe("[DNS client]", function()
-- insert a stub thats waits and returns a fixed record
local call_count = 0
local name = TEST_DOMAIN
query_func = function()
resolver.query_func = function()
local ip = "1.4.2.3"
local entry = {
{
Expand Down
27 changes: 16 additions & 11 deletions spec/01-unit/21-dns-client/03-client_cache_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,32 @@ end

describe("[DNS client cache]", function()

local client, resolver, query_func
local client, resolver

before_each(function()
client = require("kong.resty.dns.client")
resolver = require("resty.dns.resolver")

-- you can replace this `query_func` upvalue to spy on resolver query calls.
-- `resolver.query_func` is hooked to inspect resolver query calls. New values can be assigned to it.
-- This default will just call the original resolver (hence is transparent)
query_func = function(self, original_query_func, name, options)
resolver.query_func = function(self, original_query_func, name, options)
return original_query_func(self, name, options)
end

-- patch the resolver lib, such that any new resolver created will query
-- using the `query_func` upvalue defined above
-- using the `resolver.query_func` defined above
local old_new = resolver.new
resolver.new = function(...)
local r = old_new(...)
local original_query_func = r.query

-- remember the passed in query_func
-- so it won't be replaced by the next resolver.new call
-- and won't interfere with other tests
local query_func = resolver.query_func
r.query = function(self, ...)
if not query_func then
print(debug.traceback("WARNING: query_func is not set"))
if not resolver.query_func then
print(debug.traceback("WARNING: resolver.query_func is not set"))
dump(self, ...)
return
end
Expand All @@ -53,8 +58,8 @@ describe("[DNS client cache]", function()
package.loaded["kong.resty.dns.client"] = nil
package.loaded["resty.dns.resolver"] = nil
client = nil
resolver.query_func = nil
resolver = nil
query_func = nil
end)


Expand All @@ -81,7 +86,7 @@ describe("[DNS client cache]", function()
assert(client.init(config))
lrucache = client.getcache()

query_func = function(self, original_query_func, qname, opts)
resolver.query_func = function(self, original_query_func, qname, opts)
return mock_records[qname..":"..opts.qtype] or { errcode = 3, errstr = "name error" }
end
end)
Expand Down Expand Up @@ -218,7 +223,7 @@ describe("[DNS client cache]", function()
-- the 'result3' resolve call above will also trigger a new background query
-- (because the sleep of 0.1 equals the records ttl of 0.1)
-- so let's yield to activate that background thread now. If not done so,
-- the `after_each` will clear `query_func` and an error will appear on the
-- the `after_each` will clear `resolver.query_func` and an error will appear on the
-- next test after this one that will yield.
sleep(0.1)
end)
Expand Down Expand Up @@ -281,7 +286,7 @@ describe("[DNS client cache]", function()
assert(client.init(config))
lrucache = client.getcache()

query_func = function(self, original_query_func, qname, opts)
resolver.query_func = function(self, original_query_func, qname, opts)
return mock_records[qname..":"..opts.qtype] or { errcode = 3, errstr = "name error" }
end
end)
Expand Down Expand Up @@ -461,7 +466,7 @@ describe("[DNS client cache]", function()
assert(client.init(config))
lrucache = client.getcache()

query_func = function(self, original_query_func, qname, opts)
resolver.query_func = function(self, original_query_func, qname, opts)
return mock_records[qname..":"..opts.qtype] or { errcode = 3, errstr = "name error" }
end
end)
Expand Down

1 comment on commit 4bd17e1

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bazel Build

Docker image available kong/kong:4bd17e1afbff7e2de0243fad6b49d2e7a16a59dc
Artifacts available https://github.com/Kong/kong/actions/runs/9740768795

Please sign in to comment.