Conversation
When finalCacheOnly is enabled and additional_section is true (default), the parseAnswer() function incorrectly keeps A/AAAA records from the DNS Additional section (section=3) alongside Answer section records. It then overwrites their names to the queried domain name, causing wrong IPs (belonging to nameserver glue records) to be returned for the queried domain. The fix adds a section check in the finalCacheOnly block to exclude Additional section (section=3) records from both the result set and the min_ttl calculation. These records are glue records for nameservers and should never be treated as answers for the queried domain. Root cause chain: 1. additional_section=true brings Section 3 glue records into the flat answers list 2. finalCacheOnly filters by type only (not name or section) 3. Line 674 overwrites all kept records' names to check_qname, making Section 3 records appear to belong to the queried domain 4. The subsequent name-based filter at line 687 no longer catches them 5. Caller receives records with correct name but wrong IP address The section=3 field preserved in output serves as evidence of the bug.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWhen Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Author
|
The CI failure in All busted spec tests (including the 3 new |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/client_cache_spec.lua (1)
670-701: Consider strengthening the edge-case test assertions.The conditional assertion at lines 694-700 is defensive but doesn't clearly verify the expected behavior. It allows
resultto benilwithout checkingerr, and ifresultexists, it only checks that no section=3 records are present.Consider making the test more explicit about the expected outcome:
♻️ Suggested improvement for clearer assertions
-- Should not return the section 3 A record as an answer local result, err = client.resolve("myservice", { qtype = client.TYPE_A }) - if result then - -- If any result is returned, it must not be the section 3 record - for _, r in ipairs(result) do - assert.not_equal(3, r.section) - assert.not_equal("10.0.0.11", r.address) - end - end + -- With only CNAME in Answer section and no A record to dereference, + -- resolution should fail or return empty/CNAME only + if result and `#result` > 0 then + -- If any result is returned, it must not be the section 3 record + for _, r in ipairs(result) do + assert.not_equal(3, r.section) + assert.not_equal("10.0.0.11", r.address) + end + else + -- Expected: either nil result or empty result since there's no + -- resolvable A record in the Answer section + assert.truthy(result == nil or `#result` == 0 or result[1].type == client.TYPE_CNAME) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/client_cache_spec.lua` around lines 670 - 701, Make the test explicit about expected outcomes by asserting error absence and tightening result checks: call client.resolve("myservice", { qtype = client.TYPE_A }), then immediately assert.is_nil(err); if result is nil assert.is_nil(result) (or assert.is_falsy(result)) to confirm no unexpected data, otherwise iterate result and assert no record has section == 3 and no record.address == "10.0.0.11"; optionally also assert that any returned answer is the CNAME (type == client.TYPE_CNAME) to verify the answer section behavior. Use the existing local variables result and err and reference client.resolve and mock_records in the updated assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/client_cache_spec.lua`:
- Around line 670-701: Make the test explicit about expected outcomes by
asserting error absence and tightening result checks: call
client.resolve("myservice", { qtype = client.TYPE_A }), then immediately
assert.is_nil(err); if result is nil assert.is_nil(result) (or
assert.is_falsy(result)) to confirm no unexpected data, otherwise iterate result
and assert no record has section == 3 and no record.address == "10.0.0.11";
optionally also assert that any returned answer is the CNAME (type ==
client.TYPE_CNAME) to verify the answer section behavior. Use the existing local
variables result and err and reference client.resolve and mock_records in the
updated assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1b7f84c-47bc-4ff7-8f49-07d9050878b7
📒 Files selected for processing (2)
spec/client_cache_spec.luasrc/resty/dns/client.lua
nic-6443
reviewed
Apr 1, 2026
Per review feedback, using == 1 (Answer section) is more precise than != 3 (not Additional section). We only want Answer section records in the finalCacheOnly result set.
t/02-timer-usage.t relied on external DNS resolution (8.8.8.8 for httpbin.org and mockbin.org) which fails in CI environments without external network access. Changes: - Switch to local CoreDNS server (127.0.0.1:15353) with test domains svc1.test and svc2.test - Add CoreDNS template entries for the test domains - Use consistent client.init() config in both init_worker and access phases (the access_by_lua block previously called client.init() without nameserver config, falling back to system resolv.conf) - Add early return after resolve failure to prevent nil dereference
nic-6443
approved these changes
Apr 1, 2026
AlinsRan
approved these changes
Apr 1, 2026
membphis
approved these changes
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
finalCacheOnlyis enabled (used by api7-ee-3-gateway) andadditional_sectionistrue(default inresolve()), theparseAnswer()function incorrectly returns DNS Additional section (section=3) glue records as answers for the queried domain.Root Cause
The
finalCacheOnlyblock inparseAnswer()(L658-678) filters records by type only, not by name or section:additional_section=truebrings Section 3 glue records (A records for nameservers) into the flatanswerslistfinalCacheOnlykeeps all records matchingqtype(e.g., TYPE_A) — including Section 3 A records that belong to different domainscheck_qname, making Section 3 records appear to belong to the queried domainSymptoms
section: 3(proof of wrong origin)min_ttlacross all sections, producing unexpected TTL valuesmath.random()selects from all kept records, sometimes picking the correct oneFix
Added a
section == 1(Answer section) check in thefinalCacheOnlyblock to exclude non-Answer section records from both the result set and themin_ttlcalculation. These are glue records for nameservers and should never be treated as answers for the queried domain.CI Fix
Fixed pre-existing
t/02-timer-usage.tfailure: the test relied on external DNS resolution (8.8.8.8forhttpbin.organdmockbin.org) which fails in CI environments. Switched to use local CoreDNS server (127.0.0.1:15353) with test domainssvc1.testandsvc2.test. Also fixed a nil dereference bug in the test where resolve failure did not early-return before accessing the result.Test Coverage
Added 3 test cases for
finalCacheOnlywith Additional section records: