Fix Using ws:// With Self Hosted
#269
Conversation
WalkthroughThis pull request introduces modifications to the WebSocket handling and API URL construction in the codebase. Key changes include updates to 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: 1
🧹 Outside diff range and nitpick comments (2)
pkg/api/version/version.go (1)
40-42: Consider adding security warning for ws:// usage.The regex pattern correctly handles both
ws://andwss://protocols, which aligns with the PR objective for self-hosted support. However, consider adding a warning log whenws://is used to ensure users are aware of the security implications.if apiType == APITypeLive || apiType == APITypeSpeakStream { r = regexp.MustCompile(`^(wss?)://(.+)$`) + // Add warning for insecure WebSocket usage + if strings.HasPrefix(host, "ws://") { + klog.Warningf("Using insecure WebSocket protocol (ws://) - This is not recommended for production environments") + } }pkg/client/common/v1/websocket.go (1)
Line range hint
553-595: Consider error handling in close sequence.While the close sequence is well-structured, the error handling in
closeStreamandnormalClosureis silently ignored with_ = c.closeStream(false). Consider logging these errors at debug level for troubleshooting connection closure issues.- _ = c.closeStream(false) + if err := c.closeStream(false); err != nil { + klog.V(6).Infof("Error during stream closure: %v\n", err) + } - _ = c.normalClosure(false) + if err := c.normalClosure(false); err != nil { + klog.V(6).Infof("Error during normal closure: %v\n", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/api/version/version.go(3 hunks)pkg/client/common/v1/websocket.go(23 hunks)pkg/client/speak/v1/websocket/client_callback.go(1 hunks)pkg/client/speak/v1/websocket/client_channel.go(1 hunks)
🔇 Additional comments (10)
pkg/api/version/version.go (2)
101-101: LGTM! Logging improvements enhance debugging capabilities.
The added logging statements provide valuable visibility into the URL construction process, using appropriate verbosity levels consistent with the rest of the file.
Also applies to: 110-110
Line range hint 40-110: Verify WebSocket protocol handling across the codebase.
Let's ensure consistent protocol handling across the codebase.
✅ Verification successful
Let me gather more context about the WebSocket protocol handling by checking the implementation details.
WebSocket protocol handling is consistent and secure across the codebase.
The implementation shows a well-structured and secure approach to WebSocket protocol handling:
- The protocol is properly defined as a constant
WSProtocol = "wss"(secure WebSocket) - The WebSocket client implementation correctly handles both
ws://andwss://protocols with appropriate TLS configuration - Protocol selection is consistent with the API type (
APITypeLiveorAPITypeSpeakStream) - The client includes proper connection security with TLS configuration and authentication headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for WebSocket protocol handling and any hardcoded protocols
# Test 1: Check for WebSocket protocol handling
echo "Checking WebSocket protocol handling..."
rg -l "ws://" || rg -l "wss://"
# Test 2: Check for protocol-related constants or variables
echo "Checking protocol-related definitions..."
rg -i "protocol.*=.*ws" || rg -i "protocol.*=.*wss"
# Test 3: Check for URL construction with WebSocket protocols
echo "Checking URL construction with WebSocket protocols..."
ast-grep --pattern 'url.URL{Scheme: $protocol'
Length of output: 526
Script:
#!/bin/bash
# Check the WebSocket protocol implementation details
# Show the constants file content
echo "Checking constants definition..."
cat pkg/api/version/constants.go
# Show the websocket client implementation
echo -e "\nChecking WebSocket client implementation..."
cat pkg/client/common/v1/websocket.go
# Check for any other protocol-related code
echo -e "\nChecking other protocol usage..."
rg "WSProtocol" -A 3 -B 3
Length of output: 18489
pkg/client/speak/v1/websocket/client_callback.go (1)
Line range hint 63-69: Verify WebSocket protocol handling for self-hosted setups.
While the logging improvements are good, let's verify the main PR objective regarding ws:// protocol handling:
#!/bin/bash
# Search for WebSocket URL/protocol handling logic
echo "Checking WebSocket protocol handling implementation:"
rg -l "ws://"
echo -e "\nChecking URL construction logic:"
ast-grep --pattern 'func GetSpeakStreamAPI($$$) {
$$$
}'pkg/client/speak/v1/websocket/client_channel.go (2)
92-92: LGTM! Logging message correction improves clarity.
The updated error message now correctly reflects that the error occurred in the binary message handling (router.Binary) rather than generic message handling, making the logs more accurate and easier to debug.
Line range hint 63-70: Verify WebSocket URL protocol handling for self-hosted configurations.
Since this PR aims to fix ws:// protocol handling for self-hosted configurations, we should verify that the URL construction through version.GetSpeakStreamAPI correctly handles both ws:// and wss:// protocols.
pkg/client/common/v1/websocket.go (5)
193-208: LGTM: Proper handling of ws:// protocol.
The implementation correctly handles both secure and non-secure WebSocket connections by configuring the dialer based on the URL scheme. For ws:// URLs, TLS is disabled, while for wss:// URLs, TLS is enabled with the configured server authentication settings.
102-106: LGTM: Improved mutex handling in error paths.
The code now consistently unlocks the mutex in all error paths, preventing potential deadlocks. This is a good improvement to the error handling logic.
Also applies to: 115-119, 134-138, 185-188, 227-230, 251-256
Line range hint 263-393: LGTM: Comprehensive error handling in WebSocket listener.
The error handling is well-structured and covers all important cases:
- Panic recovery with proper cleanup
- Graceful socket closure detection
- Fatal socket errors with appropriate error propagation
- Deepgram-specific error handling
- EOF conditions
467-469: LGTM: Enhanced debugging with payload logging.
The addition of payload logging at debug level helps with troubleshooting while maintaining security by using appropriate log levels.
193-208: Verify the WebSocket protocol handling in self-hosted environments.
The changes correctly implement the PR objective of fixing ws:// protocol usage. To ensure the changes work as expected in self-hosted environments:
- Test with both
ws://andwss://URLs - Verify TLS settings are correctly applied based on the URL scheme
✅ Verification successful
Let me gather more information about the URL construction and WebSocket configuration to ensure comprehensive verification.
WebSocket protocol handling implementation is correctly implemented
The verification confirms that:
- The WebSocket URL scheme detection (
ws://) is properly implemented inpkg/client/common/v1/websocket.go - TLS configuration is correctly disabled for
ws://and enabled for other schemes (implicitlywss://) - URL construction is consistently handled through proper getter methods across different client implementations
- The
SkipServerAuthoption is properly propagated to both WebSocket and TLS configurations
The changes successfully achieve the PR objective of supporting ws:// protocol in self-hosted environments while maintaining secure defaults for other cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the WebSocket URL handling across the codebase
echo "Checking WebSocket URL construction and usage..."
rg -A 5 "ws://" || echo "No hardcoded ws:// URLs found (good)"
rg -A 5 "wss://" || echo "No hardcoded wss:// URLs found (good)"
# Check for proper URL construction
ast-grep --pattern 'GetURL($$$)'
Length of output: 774
Script:
#!/bin/bash
# Check for URL construction and WebSocket configuration patterns
echo "Checking URL construction patterns..."
ast-grep --pattern 'url := $$$'
echo "\nChecking WebSocket configuration and options..."
ast-grep --pattern 'type $_ struct {
$$$
SkipServerAuth bool
$$$
}'
echo "\nChecking for TLS configuration patterns..."
rg -A 5 "TLSClientConfig"
Length of output: 1912
Proposed changes
This fixes an issue with using
ws://and Self-Hosted Configurations.Also:
Types of changes
What types of changes does your code introduce to the community Go SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
NA
Summary by CodeRabbit
WSCallbackandWSChannelstructures.