filter answer section only

[dogancanbakir]

closes #923

Summary by CodeRabbit

• New Features

  • DNS filtering now strictly uses answer-section records, reducing false positives from additional-section entries.

• Tests

  • Integration and unit tests expanded to cover many DNS record types, added scenarios for edge cases and robustness.

• Chores

  • DNS processing updated to apply answer-section filtering earlier for more consistent downstream results.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a filter that retains only ANSWER-section resource records in DNS responses, integrates it into the runner pipeline, and adds unit and integration tests to ensure AUTHORITY/ADDITIONAL-section records (e.g., root-server A records) are not treated as target answers.

Changes

Cohort / File(s)	Summary
Core filtering logic libs/dnsx/util.go	New exported function FilterAnswerSectionOnly(dnsData *retryabledns.DNSData) that keeps only RRs from the ANSWER section, clears other sections when appropriate, rebuilds AllRecords, normalizes values where needed, preserves SOA NS/Mbox/Serial, and is nil-safe.
Runner integration internal/runner/runner.go	Calls dnsx.FilterAnswerSectionOnly(dnsData.DNSData) in the worker loop immediately after DNS data is obtained and before host/timestamp validation or output.
Unit tests libs/dnsx/util_test.go	Adds TestFilterAnswerSectionOnly covering: removal of ADDITIONAL-section A records, retaining ANSWER-section A records, extracting CNAME from ANSWER, handling nil RawResp, and handling nil DNSData without panics.
Integration tests cmd/integration-test/dns.go	Adds dnsFilterAdditionalSection test type and dnshandlerWithAdditional handler to simulate AUTHORITY/ADDITIONAL sections and verify filtering of unintended root-server IPs; integrates new test case into dnsTestcases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Focus areas:
  • Correct handling and normalization for each RR type in FilterAnswerSectionOnly
  • Accurate reconstruction of AllRecords and preservation of SOA fields
  • Nil-safety (nil dnsData, nil RawResp)
  • Correct placement of the filter call in runner.go to ensure it runs before validation/output
  • Integration test DNS message construction and expectations

Poem

🐰 I hopped through packets, peered at each reply,

Kept only ANSWERs, waved the extras goodbye,
No root hops sneaking into the host array,
Tests snug and tidy — I bounded away,
🥕 Hop, filter, celebrate!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'filter answer section only' directly summarizes the main change, which adds filtering functionality to isolate DNS ANSWER section records.
Linked Issues check	✅ Passed	The PR successfully addresses issue #923 by implementing FilterAnswerSectionOnly function, integrating it into the runner workflow, and adding comprehensive tests to filter DNS records to ANSWER section only.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to implementing the answer-section-only filtering feature: new filter function, integration into runner, new test integration test flow, and comprehensive unit tests.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 923_filter_answer_section_only

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

cmd/integration-test/dns.go (2)

11-15: Integration test correctly reproduces the ADDITIONAL-section bug scenario

The new "DNS Filter Additional Section" testcase and dnsFilterAdditionalSection.Execute reliably simulate a response with only NS/ADDITIONAL A records and assert that root-server IPs are not present in dnsx output. This is a good end-to-end guard for the regression.

One optional enhancement: if the intended behavior is “no answers at all” for such a query, you could additionally assert len(results) == 0 to make that expectation explicit.

Also applies to: 138-175

---

138-141: Unused expectedOutput field in dnsFilterAdditionalSection

expectedOutput is defined on dnsFilterAdditionalSection but never used in Execute. Since the test only checks for the absence of specific IPs, this field can be safely removed to reduce noise, unless you plan to extend the assertions later.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31932fd and 05b1645.

📒 Files selected for processing (4)

• cmd/integration-test/dns.go (2 hunks)
• internal/runner/runner.go (1 hunks)
• libs/dnsx/util.go (2 hunks)
• libs/dnsx/util_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

internal/runner/runner.go (1)

libs/dnsx/util.go (1)

• FilterAnswerSectionOnly (45-128)

libs/dnsx/util_test.go (1)

libs/dnsx/util.go (1)

• FilterAnswerSectionOnly (45-128)

cmd/integration-test/dns.go (1)

internal/testutils/integration.go (1)

• RunDnsxAndGetResults (11-36)

🪛 GitHub Check: Lint Test

cmd/integration-test/dns.go

[failure] 225-225:
Error return value of w.WriteMsg is not checked (errcheck)

---

[failure] 153-153:
Error return value of srv.Shutdown is not checked (errcheck)

---

[failure] 152-152:
Error return value of srv.ListenAndServe is not checked (errcheck)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: release-test
• GitHub Check: Functional Test (macOS-latest)
• GitHub Check: Functional Test (windows-latest)
• GitHub Check: Analyze (go)

🔇 Additional comments (3)

internal/runner/runner.go (1)

635-643: Good placement of FilterAnswerSectionOnly in the worker pipeline

Calling dnsx.FilterAnswerSectionOnly(dnsData.DNSData) right after QueryMultiple and the nil check ensures all subsequent consumers (JSON output, record-type printing, wildcard storage, ASN/CDN lookups) operate on ANSWER-only data. The added nil guard inside the helper keeps this safe.

libs/dnsx/util.go (1)

43-127: ANSWER-only filtering logic looks correct and robust

The implementation cleanly rebuilds all record slices from RawResp.Answer only, with appropriate lowercasing and SOA struct preservation, and safely handles nil and empty-ANSWER cases. This matches the objective of excluding AUTHORITY/ADDITIONAL data from downstream use.

libs/dnsx/util_test.go (1)

11-137: Unit test coverage for FilterAnswerSectionOnly is solid

The table-driven tests cover the key edge cases (ADDITIONAL-only A records, mixed ANSWER/ADDITIONAL, CNAME-only ANSWER, nil RawResp, nil DNSData) and align with the intended semantics of using ANSWER-only data.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

cmd/integration-test/dns.go (1)

153-154: //nolint usage resolves errcheck; consider narrowing to //nolint:errcheck

Adding //nolint to srv.ListenAndServe(), srv.Shutdown(), and w.WriteMsg(&msg) addresses the previous errcheck lint failures.

If your project convention prefers targeting only the specific linter (as suggested in the earlier review), you may want to switch these to //nolint:errcheck so you don’t accidentally suppress other linters on the same lines:

-go srv.ListenAndServe() //nolint
-defer srv.Shutdown()    //nolint
+go srv.ListenAndServe() //nolint:errcheck
+defer srv.Shutdown()    //nolint:errcheck

- w.WriteMsg(&msg) //nolint
+ w.WriteMsg(&msg) //nolint:errcheck

Purely stylistic/precision; functionally things are fine as-is.

Also applies to: 236-236

🧹 Nitpick comments (3)

cmd/integration-test/dns.go (3)

4-4: Strengthen the JSON-based assertion in the new “DNS Filter Additional Section” test

The overall structure (local DNS server with only NS/Additional A records, running dnsx with -a -resp -json, and scanning JSON for root IPs) matches the bug report and should catch the original regression.

Two small robustness gaps you might want to tighten:

• The test passes when len(results) == 0 or when json.Unmarshal fails (you continue), so if dnsx stops emitting JSON or changes its output format, this test will silently succeed instead of flagging a failure.
• Given the scenario (no A answers at all), it would be stronger to assert that the "a" field is either absent or empty, not just that it does not contain those three specific root IPs.

A possible refinement would be to:

• Fail if no JSON line is successfully parsed, and
• When "a" is present, additionally assert len(aField) == 0.

That keeps the intent clear and makes the test more future-proof while still validating that ADDITIONAL-section IPs never leak into "a".

Also applies to: 13-16, 144-186

---

172-180: Avoid hard type assertion in JSON parsing loop

ipStr := strings.ToLower(ip.(string)) will panic if the "a" field ever stops being an array of strings (schema change, bug, or malformed output). That would bring down the whole integration test run with a panic instead of a clean assertion failure.

You could guard this cheaply:

- for _, ip := range aField {
-   ipStr := strings.ToLower(ip.(string))
+ for _, ip := range aField {
+   s, ok := ip.(string)
+   if !ok {
+     return errIncorrectResult("expected string IP in 'a' field", result)
+   }
+   ipStr := strings.ToLower(s)

This keeps the test strict about schema while failing in a controlled way instead of panicking.

---

188-237: Handler for NS/Additional records looks correct; consider a small defensive check

The new dnshandlerWithAdditional correctly:

• Validates the question name before responding.
• Returns an authoritative reply with an empty Answer, NS records in msg.Ns, and root A records in msg.Extra.
• Uses w.WriteMsg(&msg) to emit the crafted response.

That matches the scenario needed to reproduce the bug.

If you want to make this a bit more defensive without changing behavior, you could add a quick guard before accessing r.Question[0]:

if len(r.Question) == 0 {
    return
}

Not required for the current tests (which always send a normal query), but it makes the handler safer if it’s ever reused.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 532c626 and 398afb7.

📒 Files selected for processing (2)

• cmd/integration-test/dns.go (3 hunks)
• libs/dnsx/util_test.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• libs/dnsx/util_test.go

🧰 Additional context used

🧬 Code graph analysis (1)

cmd/integration-test/dns.go (1)

internal/testutils/integration.go (1)

• RunDnsxAndGetResults (11-36)

[nil0x42]

IMHO It doesn't fix the issue, please take a look at my review:
#923 (comment)

The filter should really parse from raw response & filter data from ANSWER SECTION only (at least for A records). Only records mentionned in the SECTION should appear in the list of found A records

[Mzack9999]

Let's review the reported edge cases at #923 (comment)