ws proxy support #2308
ws proxy support #2308
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (2)
node/pkg/websocketfetcher/app.go (2)
201-202: Add validation and documentation for WS_PROXY environment variable.Consider adding validation for the proxy URL format and documenting the expected format (e.g.,
protocol://host:port). This will help prevent configuration errors and improve security.Example validation:
wsProxy := os.Getenv("WS_PROXY") +if wsProxy != "" { + if _, err := url.Parse(wsProxy); err != nil { + log.Warn().Str("proxy", wsProxy).Msg("invalid WS_PROXY URL format") + } +}
Line range hint
201-212: Consider enhancing proxy configuration flexibility.The current implementation applies the same proxy to all CEX fetchers. Consider these improvements:
- Allow per-provider proxy configuration for better flexibility
- Add debug logging when a proxy is configured
- Consider implementing proxy health checks
Example enhancement:
wsProxy := os.Getenv("WS_PROXY") +if wsProxy != "" { + log.Debug().Str("proxy", wsProxy).Msg("WebSocket proxy configured") +} for name, factory := range appConfig.CexFactories { if _, ok := feedMap[name]; !ok { log.Warn().Msgf("no feeds for %s", name) continue } + // Allow per-provider proxy override + providerProxy := os.Getenv(fmt.Sprintf("WS_PROXY_%s", strings.ToUpper(name))) + if providerProxy == "" { + providerProxy = wsProxy + } fetcher, err := factory( ctx, common.WithFeedDataBuffer(a.buffer), common.WithFeedMaps(feedMap[name]), - common.WithProxy(wsProxy), + common.WithProxy(providerProxy), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
node/pkg/websocketfetcher/app.go(3 hunks)node/pkg/websocketfetcher/common/type.go(0 hunks)node/pkg/wss/utils.go(2 hunks)
💤 Files with no reviewable changes (1)
- node/pkg/websocketfetcher/common/type.go
🔇 Additional comments (3)
node/pkg/wss/utils.go (2)
8-8: LGTM: Import addition is appropriate
The strings package import is required for the URL scheme manipulation in the Dial method.
20-22:
Consider improving the WebSocket URL scheme handling
While the scheme modification is a common pattern for proxy support, there are several concerns to address:
- The current implementation modifies
ws.Endpointdirectly, which could cause issues if the connection needs to be re-established without a proxy later. - The scheme change from WSS to WS should be logged for debugging purposes.
- The resulting URL should be validated after modification.
- Security implications of downgrading from WSS to WS should be documented.
Consider applying these improvements:
if ws.Proxy != "" {
+ originalEndpoint := ws.Endpoint
if strings.HasPrefix(ws.Endpoint, "wss") {
- ws.Endpoint = strings.Replace(ws.Endpoint, "wss", "ws", 1)
+ modifiedEndpoint := strings.Replace(ws.Endpoint, "wss", "ws", 1)
+ if _, err := url.Parse(modifiedEndpoint); err != nil {
+ return fmt.Errorf("invalid URL after scheme modification: %v", err)
+ }
+ ws.Endpoint = modifiedEndpoint
+ log.Info().
+ Str("original_url", originalEndpoint).
+ Str("modified_url", ws.Endpoint).
+ Msg("downgraded WSS to WS for proxy support")
}
}Let's verify if this URL scheme modification is a common pattern in the codebase:
node/pkg/websocketfetcher/app.go (1)
Line range hint 201-212: Verify WebSocket proxy implementation and error handling.
Let's verify the proxy implementation in related files to ensure proper error handling and connection management.
✅ Verification successful
Based on the comprehensive results from the shell scripts, I can now provide the final response:
WebSocket proxy implementation and error handling are properly implemented.
The codebase shows a robust implementation of proxy support with proper error handling:
- All WebSocket providers consistently implement proxy configuration via
WithProxyUrl - Error handling is properly implemented at multiple levels:
- Connection initialization with proxy in
wss.Dial - Provider-level error handling in each provider's
New()function - Consistent error logging with context in all providers
- Connection initialization with proxy in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check WebSocket proxy implementation and error handling
# Test 1: Check for proper error handling in WebSocket connection code
echo "Checking WebSocket connection error handling..."
rg -A 5 "WithProxy|Dial.*error" --type go
# Test 2: Check for proxy usage implementation
echo "Checking proxy usage implementation..."
ast-grep --pattern 'func $_(ctx context.Context, opts ...common.FetcherOption) (common.FetcherInterface, error) {
$$$
}'
Length of output: 90164
Description
utilize ws proxy to reduce network pressure
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment