Use proxy for dns and ssl templates #6255
Conversation
- while using template execute level function we need to override custom dialer
WalkthroughThe changes introduce support for overriding the default dialer in network-related protocol executors by propagating an Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as ExecutorOptions
participant Protocol as Protocol Request
participant Pool as NetworkClientPool
Executor->>Protocol: Pass OverrideFastdialer in options
Protocol->>Pool: Get(config{CustomDialer: options.OverrideFastdialer})
alt CustomDialer is set
Pool-->>Protocol: Return CustomDialer
else
Pool-->>Protocol: Return normalClient
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/protocols/dns/dnsclientpool/clientpool.go(1 hunks)pkg/protocols/network/network.go(1 hunks)pkg/protocols/network/networkclientpool/clientpool.go(2 hunks)pkg/protocols/protocols.go(2 hunks)pkg/protocols/ssl/ssl.go(2 hunks)pkg/protocols/websocket/websocket.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
pkg/protocols/websocket/websocket.go (2)
pkg/protocols/network/networkclientpool/clientpool.go (2)
Get(34-41)Configuration(24-26)pkg/types/types.go (1)
Options(30-452)
pkg/protocols/protocols.go (1)
pkg/protocols/common/protocolstate/state.go (1)
Dialer(24-24)
pkg/protocols/dns/dnsclientpool/clientpool.go (1)
pkg/types/types.go (1)
Options(30-452)
pkg/protocols/network/network.go (1)
pkg/protocols/network/networkclientpool/clientpool.go (2)
Get(34-41)Configuration(24-26)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (8)
pkg/protocols/protocols.go (2)
8-8: LGTM! Clean import addition.The fastdialer import is appropriately added to support the new OverrideFastdialer field.
136-137: LGTM! Well-designed field addition.The OverrideFastdialer field is well-named, properly typed, and includes clear documentation. This provides a clean way to override the default dialer across protocol implementations.
pkg/protocols/dns/dnsclientpool/clientpool.go (1)
86-90: LGTM! Improved constructor with proxy support.The change from positional parameters to a structured
Optionsapproach is a good improvement. The addition of proxy support viaoptions.AliveSocksProxyaligns with the PR objective to enable proxy functionality for DNS templates.pkg/protocols/websocket/websocket.go (1)
103-105: LGTM! Consistent implementation of dialer override.The change correctly passes the
OverrideFastdialerthrough thenetworkclientpool.Configuration, maintaining consistency with other protocol implementations in this PR.pkg/protocols/network/networkclientpool/clientpool.go (2)
24-26: LGTM! Clean configuration struct addition.The
OverrideDialerfield is appropriately typed and provides a clean way to pass custom dialers through the configuration.
36-38: LGTM! Proper override pattern implementation.The logic correctly returns the override dialer when provided, with appropriate null safety checks. The fallback to
normalClientmaintains backward compatibility.pkg/protocols/network/network.go (1)
240-242: LGTM! Consistent dialer override implementation.The change correctly integrates with the network client pool's new configuration-based approach, passing the
OverrideFastdialerwhen available. This maintains consistency with the pattern established across other protocol implementations.pkg/protocols/ssl/ssl.go (1)
118-120: LGTM: Proxy support correctly implemented.The change properly passes the
OverrideFastdialerfrom executor options to the network client pool, enabling proxy functionality for SSL templates. This aligns with the PR objective to support proxy usage in DNS and SSL templates.
- proxy client is shared between non proxy requests
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/protocols/dns/dns.go (1)
190-204: 🛠️ Refactor suggestionDuplicate
len(request.Resolvers)guard wipes the resolver listThe inner
if len(request.Resolvers) > 0 { … }repeats the outer guard and appends any expression-evaluated resolvers todnsClientOptions.Resolvers, but the assignment right after itdnsClientOptions.Resolvers = request.Resolversoverwrites the previously built slice, effectively discarding the evaluated items.
- if len(request.Resolvers) > 0 { - for _, resolver := range request.Resolvers { - … - dnsClientOptions.Resolvers = append(dnsClientOptions.Resolvers, resolver) - } - } - dnsClientOptions.Resolvers = request.Resolvers + for _, resolver := range request.Resolvers { + … + dnsClientOptions.Resolvers = append(dnsClientOptions.Resolvers, resolver) + }Removing the redundant outer check and not resetting
dnsClientOptions.Resolverspreserves the resolved/templated entries.
This is pre-existing code but it lies in the modified hunk; fixing it now prevents subtle resolver-selection bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/dns/dns.go(1 hunks)pkg/protocols/dns/dnsclientpool/clientpool.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/dns/dnsclientpool/clientpool.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (ubuntu-latest)
- GitHub Check: Tests (windows-latest)
| dnsClientOptions := &dnsclientpool.Configuration{ | ||
| Retries: request.Retries, | ||
| Proxy: options.Options.AliveSocksProxy, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pass the override dialer alongside the proxy for full template-level control
Other protocol implementations in this PR (network, ssl, websocket) forward options.OverrideFastdialer so that a template can completely replace the default fast-dialer.
DNS now only forwards the SOCKS proxy string which leaves the dialer override path unused for DNS templates and breaks the symmetry promised in the PR description.
dnsClientOptions := &dnsclientpool.Configuration{
Retries: request.Retries,
Proxy: options.Options.AliveSocksProxy,
+ // Keep behaviour consistent with other protocol pools
+ CustomDialer: options.OverrideFastdialer,
}Please verify that dnsclientpool.Configuration already exposes a CustomDialer (or similar) field; if not, extend it as was done for networkclientpool.
Failing to forward the dialer means DNS templates still funnel through the global fast-dialer even when the user explicitly asked for an override.
🏁 Script executed:
#!/bin/bash
# Inspect dnsclientpool package for Configuration and CustomDialer
grep -R "package dnsclientpool" -n pkg/protocols/dns
grep -R "type Configuration" -n pkg/protocols/dns
grep -R "CustomDialer" -n pkg/protocols/dns
# Compare with networkclientpool implementation
grep -R "package networkclientpool" -n pkg/protocols/network
grep -R "type Configuration" -n pkg/protocols/network
grep -R "CustomDialer" -n pkg/protocols/networkLength of output: 1021
Extend DNS client configuration to support dialer overrides
To restore symmetry with other protocol pools, add a CustomDialer field to the DNS client configuration and forward the override in dns.go:
• In pkg/protocols/dns/dnsclientpool/clientpool.go, modify the Configuration struct:
type Configuration struct {
Retries int
Proxy string
+ CustomDialer *fastdialer.Dialer
}(Remember to import the fastdialer package.)
• In pkg/protocols/dns/dns.go, update the options passed into the pool:
dnsClientOptions := &dnsclientpool.Configuration{
Retries: request.Retries,
Proxy: options.Options.AliveSocksProxy,
+ CustomDialer: options.OverrideFastdialer,
}This ensures DNS templates can fully override the default fast-dialer just like network, SSL, and WebSocket.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dnsClientOptions := &dnsclientpool.Configuration{ | |
| Retries: request.Retries, | |
| Proxy: options.Options.AliveSocksProxy, | |
| } | |
| dnsClientOptions := &dnsclientpool.Configuration{ | |
| Retries: request.Retries, | |
| Proxy: options.Options.AliveSocksProxy, | |
| // Keep behaviour consistent with other protocol pools | |
| CustomDialer: options.OverrideFastdialer, | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/dns/dns.go around lines 186 to 189, the
dnsclientpool.Configuration is only set with Retries and Proxy, but it does not
forward the OverrideFastdialer option. To fix this, first verify and if missing,
add a CustomDialer field to the Configuration struct in
pkg/protocols/dns/dnsclientpool/clientpool.go, importing the fastdialer package
as needed. Then, update the dnsClientOptions initialization in dns.go to include
options.OverrideFastdialer assigned to the new CustomDialer field, ensuring DNS
templates can override the default fast-dialer consistently with other
protocols.
There was a problem hiding this comment.
@dogancanbakir Teşekkürler, onayınızı aldım. DNS için de CustomDialer’ı iletip diğer protokollerle tutarlılık sağlayacak değişiklikleri planlıyorum. Bir sonraki adımda dnsclientpool.Configuration yapısına alan eklenecek ve dns.go içinde options.OverrideFastdialer aktarılacak. Bunu planlayarak ilerleyeceğim.
There was a problem hiding this comment.
Just to clarify, are you recommending that I include it even though it isn’t required?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| dnsClientOptions := &dnsclientpool.Configuration{ | ||
| Retries: request.Retries, | ||
| Proxy: options.Options.AliveSocksProxy, | ||
| } |
| @@ -132,6 +133,8 @@ type ExecutorOptions struct { | |||
| ExportReqURLPattern bool | |||
| // GlobalMatchers is the storage for global matchers with http passive templates | |||
| GlobalMatchers *globalmatchers.Storage | |||
| // OverrideFastdialer is a fastdialer dialer instance | |||
| OverrideFastdialer *fastdialer.Dialer | |||
There was a problem hiding this comment.
is this for lib usage only?
- use request.dialer instead of global variable
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/protocols/http/http.go(4 hunks)pkg/protocols/http/request.go(2 hunks)pkg/protocols/network/request.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/protocols/http/request.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/protocols/http/http.go (3)
pkg/protocols/common/protocolstate/state.go (1)
Dialer(24-24)pkg/protocols/http/httpclientpool/clientpool.go (2)
Get(177-182)Configuration(94-109)pkg/types/types.go (1)
Options(30-452)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (ubuntu-latest)
| @@ -132,6 +133,8 @@ type ExecutorOptions struct { | |||
| ExportReqURLPattern bool | |||
| // GlobalMatchers is the storage for global matchers with http passive templates | |||
| GlobalMatchers *globalmatchers.Storage | |||
| // OverrideFastdialer is a fastdialer dialer instance | |||
| OverrideFastdialer *fastdialer.Dialer | |||
There was a problem hiding this comment.
i think CustomFastdialer would be a better naming choice/convention in this context. the OverrideXXX naming pattern tends to sound like expecting a boolean value.
Proposed changes
Summary by CodeRabbit
New Features
Refactor