fixing dns vars - #7379
Conversation
Mzack9999
added
the
Type: Bug
Neo - PR Security ReviewNo security issues found Comment |
|
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 (2)
WalkthroughgetDnsClient now builds the resolver list entry-by-entry: templated resolver expressions are evaluated only when runtime metadata (merged with per-request vars) is available, unresolved templated resolvers are skipped at compile time, and the final DNS client uses the preserved evaluated resolver list. ChangesDNS Template Resolver Resolution
Sequence Diagram(s)sequenceDiagram
participant Exec as Request.Execute
participant Vars as Vars+Metadata Merge
participant Eval as Template Evaluator
participant Pool as dnsclientpool.Get
Exec->>Vars: build resolverVars (vars + metadata)
Exec->>Eval: evaluate resolvers using resolverVars (skip unresolved if metadata nil)
Eval-->>Exec: evaluated resolver list
Exec->>Pool: dnsclientpool.Get(options with evaluated resolvers)
Pool-->>Exec: dnsClient
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/protocols/dns/dns_test.go (1)
93-98: 💤 Low valueTest verifies success but not the resolved resolver values.
The test comment mentions covering "the bug where a static resolver placed alongside a templated one used to be dropped," but the assertions only check that no error occurred and the client is non-nil. Consider adding verification that the resulting client's resolver configuration contains both
"1.1.1.1"(resolved) and"8.8.8.8"(static).That said, this would require exposing internal state or refactoring
getDnsClientto return the resolved list. If that's not straightforward, the current test still provides value as a regression guard for the happy path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/protocols/dns/dns_test.go` around lines 93 - 98, Update the test that calls request.getDnsClient to assert the resolved resolver configuration includes both the templated value "1.1.1.1" and the static "8.8.8.8": either (A) if the returned client exposes resolver info (e.g., a Resolvers field or method on the client object), add require.Contains/require.ElementsMatch assertions against client.Resolvers for "1.1.1.1" and "8.8.8.8", or (B) if it does not, refactor getDnsClient to also return the resolved slice (or provide an accessor) and then assert the returned/resolved list contains both entries; reference getDnsClient and the "test_resolver" input map when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/protocols/dns/dns_test.go`:
- Around line 93-98: Update the test that calls request.getDnsClient to assert
the resolved resolver configuration includes both the templated value "1.1.1.1"
and the static "8.8.8.8": either (A) if the returned client exposes resolver
info (e.g., a Resolvers field or method on the client object), add
require.Contains/require.ElementsMatch assertions against client.Resolvers for
"1.1.1.1" and "8.8.8.8", or (B) if it does not, refactor getDnsClient to also
return the resolved slice (or provide an accessor) and then assert the
returned/resolved list contains both entries; reference getDnsClient and the
"test_resolver" input map when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f456555-9414-46a1-a17c-52ab9a2d3840
📒 Files selected for processing (3)
pkg/protocols/dns/dns.gopkg/protocols/dns/dns_test.gopkg/protocols/dns/request.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/protocols/dns/request.go (1)
178-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrace path still uses
request.dnsClient(compile-time client).
request.dnsClientis the client built at compile time, where templated resolvers were skipped (perdns.go's deferred-resolution logic). Whenrequest.Resolverscontains template expressions,Tracewill run against a client built without the user's intended resolvers (potentially the system default), even though the maindns.Docall at line 163 correctly uses the locally-rebuiltdnsClient.Consider using the locally resolved
dnsClientfor tracing as well:♻️ Proposed change
- traceData, err = request.dnsClient.Trace(domain, request.question, request.TraceMaxRecursion) + traceData, err = dnsClient.Trace(domain, request.question, request.TraceMaxRecursion)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/protocols/dns/request.go` around lines 178 - 184, The trace path incorrectly calls request.dnsClient (the compile-time client) so when request.Resolvers contains templates it uses the wrong resolver set; update the Trace call to use the locally-built dnsClient used by dns.Do (the same variable you build before calling dns.Do) instead of request.dnsClient so trace runs against the resolved runtime client (replace request.dnsClient.Trace(...) with dnsClient.Trace(...) and ensure dnsClient is in scope when request.Trace is true).
🧹 Nitpick comments (1)
pkg/reporting/trackers/linear/jsonutil/jsonutil.go (1)
66-66: 💤 Low valueUnrelated scope —
reflect.Ptr→reflect.Pointermodernization in a PR titled "fixing dns vars".
reflect.Pointeris just an alias forreflect.Ptr(added in Go 1.18), so the change is behavior-preserving and safe. However, it's unrelated to the DNS resolver fix described in the PR objective and issue#7374— consider splitting unrelated cleanups into their own PR to keep review surface focused. Not blocking.Also applies to: 96-96, 126-126, 170-170, 179-179, 201-201, 206-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/reporting/trackers/linear/jsonutil/jsonutil.go` at line 66, The PR includes unrelated modernization changes replacing reflect.Ptr with reflect.Pointer in pkg/reporting/trackers/linear/jsonutil/jsonutil.go (several occurrences) which should be split out; revert those reflect.Pointer edits back to reflect.Ptr here (or move them into a separate cleanup PR) so this PR only contains the DNS resolver fix—search for uses of reflect.Pointer in this file and replace them with reflect.Ptr to keep this change focused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/protocols/dns/request.go`:
- Around line 178-184: The trace path incorrectly calls request.dnsClient (the
compile-time client) so when request.Resolvers contains templates it uses the
wrong resolver set; update the Trace call to use the locally-built dnsClient
used by dns.Do (the same variable you build before calling dns.Do) instead of
request.dnsClient so trace runs against the resolved runtime client (replace
request.dnsClient.Trace(...) with dnsClient.Trace(...) and ensure dnsClient is
in scope when request.Trace is true).
---
Nitpick comments:
In `@pkg/reporting/trackers/linear/jsonutil/jsonutil.go`:
- Line 66: The PR includes unrelated modernization changes replacing reflect.Ptr
with reflect.Pointer in pkg/reporting/trackers/linear/jsonutil/jsonutil.go
(several occurrences) which should be split out; revert those reflect.Pointer
edits back to reflect.Ptr here (or move them into a separate cleanup PR) so this
PR only contains the DNS resolver fix—search for uses of reflect.Pointer in this
file and replace them with reflect.Ptr to keep this change focused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed2c2431-f6b2-4410-91ce-7a2887b42435
📒 Files selected for processing (5)
pkg/fuzz/dataformat/kv.gopkg/protocols/dns/request.gopkg/protocols/network/request.gopkg/reporting/trackers/linear/jsonutil/jsonutil.gopkg/utils/utils.go
2 participants
Proposed changes
Closes #7374
Checklist
Summary by CodeRabbit