Route all traffic through SOCKS5 proxy #43
Conversation
- Atlas Admin API now uses SOCKS5 proxy for all HTTP traffic - MongoDB driver connections use SOCKS5 proxy with DNS resolution through proxy - Prefer standard mongodb:// connection strings over mongodb+srv:// to avoid SRV lookups that bypass the proxy - Add comprehensive logging for proxy configuration and connection attempts Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughAdds SOCKS5 proxy support and proxy-aware HTTP client/transport to the MongoDB connector, conditionally using the proxy or direct digest-auth path during connector initialization, and refactors the mongo driver to return structured connectionInfo and use the proxy dialer during Connect. Changes
Sequence DiagramsequenceDiagram
participant Init as Connector Init
participant Proxy as Proxy Config
participant Dialer as SOCKS5 Dialer
participant HTTP as HTTP Transport
participant Auth as Digest Auth
participant Client as Admin Client
Init->>Proxy: Check proxy enabled?
alt Proxy Enabled
Proxy->>Dialer: Build SOCKS5 dialer
Dialer-->>Proxy: Return ContextDialer
Proxy->>HTTP: Create HTTP transport (dialer, timeouts, HTTP/2)
HTTP-->>Proxy: Return transport
Proxy->>Auth: Wrap transport with Digest auth
Auth-->>Proxy: Return wrapped transport
Proxy->>Client: Create admin client (with wrapped transport)
else Proxy Disabled
Init->>Auth: Use direct Digest auth transport
Auth->>Client: Create admin client
end
Client-->>Init: Client ready / error
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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
🤖 Fix all issues with AI agents
In `@pkg/connector/mongodriver/mongo_driver.go`:
- Around line 237-262: Validate the connection string prefixes before trimming
to avoid producing malformed URIs: in the logic that returns
&connectionInfo{...} for clusterInfo.ConnectionStrings.Standard and StandardSrv,
check strings.HasPrefix(*clusterInfo.ConnectionStrings.Standard, "mongodb://")
and strings.HasPrefix(*clusterInfo.ConnectionStrings.StandardSrv,
"mongodb+srv://") respectively before calling strings.TrimPrefix; if the
expected prefix is missing, return a clear error (including clusterName and
groupID) instead of trimming blindly so connectionInfo.scheme and hosts are
always correct. Ensure both branches (Standard and StandardSrv) perform the same
validation and error handling.
🧹 Nitpick comments (1)
pkg/connector/mongoconfig/proxy.go (1)
77-91: Consider returning a new client instance instead ofhttp.DefaultClient.Returning
http.DefaultClientshares global state. If callers modify the client (e.g., add middleware), it could affect unrelated code. This is a minor concern but worth noting for defensive coding.Optional: Return a fresh client when proxy is disabled
if transport == nil { - return http.DefaultClient, nil + return &http.Client{ + Timeout: 30 * time.Second, + }, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/connector/connector.gopkg/connector/mongoconfig/proxy.gopkg/connector/mongodriver/mongo_driver.go
🔇 Additional comments (8)
pkg/connector/mongoconfig/proxy.go (3)
55-73: LGTM!The early return when the proxy is disabled correctly prevents accessing a nil dialer. The transport configuration with standard timeouts and HTTP/2 support is appropriate.
93-97: LGTM!Clean adapter implementation for wrapping a function as a
ContextDialer.
47-50: The fallback wrapper is unreachable and not an issue.The
socks.Dialerreturned byproxy.SOCKS5implementsproxy.ContextDialerand has aDialContextmethod, so the type assertion on line 43 always succeeds. The wrapper on lines 47-50 never executes in practice and can be removed.Likely an incorrect or invalid review comment.
pkg/connector/mongodriver/mongo_driver.go (3)
147-153: LGTM on nil-safety for proxy.The
Dialer()method correctly handles a nil receiver, so this call is safe even whenm.proxyis nil.However, consider adding a brief comment here noting that
Dialer()is nil-safe, as it's not immediately obvious without checking the implementation.
163-220: LGTM!The retry loop correctly handles connection failures with proper cleanup (disconnecting on ping failure) and appropriate logging. The 10-attempt limit with 2-second delays is reasonable for Atlas cluster initialization.
223-226: LGTM!The
connectionInfostruct cleanly separates scheme and hosts, enabling secure URI construction with credentials in the caller.pkg/connector/connector.go (2)
154-190: LGTM!The conditional proxy configuration is well-structured:
- Properly checks for nil before accessing
mProxy- Correctly wraps the SOCKS5 transport with Digest authentication
- Falls back to standard Digest auth when proxy is disabled
- Consistent use of
clientModifiersin both paths
30-31: ThemProxyfield onMongoDBstruct appears unused after assignment.The proxy is passed to
mongodriver.NewMongoDriverat line 195, but themProxyfield stored at line 199 is never referenced elsewhere in this file. Consider removing the field if it's not needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Prefer Standard (mongodb://) connection string as it doesn't require DNS SRV lookups. | ||
| // This is important when using a SOCKS5 proxy since SRV lookups may not go through the proxy. | ||
| if clusterInfo.ConnectionStrings.Standard != nil && *clusterInfo.ConnectionStrings.Standard != "" { | ||
| l.Info( | ||
| "Using standard MongoDB connection string (no SRV lookup required)", | ||
| zap.String("cluster_name", clusterName), | ||
| zap.String("group_id", groupID), | ||
| ) | ||
| return &connectionInfo{ | ||
| scheme: "mongodb", | ||
| hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.Standard, "mongodb://"), | ||
| }, nil | ||
| } | ||
|
|
||
| return strings.TrimPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://"), nil | ||
| // Fall back to SRV connection string if Standard is not available | ||
| if clusterInfo.ConnectionStrings.StandardSrv != nil && *clusterInfo.ConnectionStrings.StandardSrv != "" { | ||
| l.Warn( | ||
| "Standard connection string not available, falling back to SRV (DNS lookups may not go through proxy)", | ||
| zap.String("cluster_name", clusterName), | ||
| zap.String("group_id", groupID), | ||
| ) | ||
| return &connectionInfo{ | ||
| scheme: "mongodb+srv", | ||
| hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://"), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
TrimPrefix doesn't validate the prefix exists, risking malformed URIs.
If Standard or StandardSrv contains an unexpected format (e.g., missing the expected prefix), TrimPrefix returns the original string unchanged. This could produce URIs like mongodb://mongodb://... or mongodb+srv://mongodb+srv://....
Proposed fix to validate prefix before trimming
// Prefer Standard (mongodb://) connection string as it doesn't require DNS SRV lookups.
// This is important when using a SOCKS5 proxy since SRV lookups may not go through the proxy.
if clusterInfo.ConnectionStrings.Standard != nil && *clusterInfo.ConnectionStrings.Standard != "" {
+ const mongoDBPrefix = "mongodb://"
+ standardStr := *clusterInfo.ConnectionStrings.Standard
+ if !strings.HasPrefix(standardStr, mongoDBPrefix) {
+ l.Warn("Unexpected standard connection string format",
+ zap.String("connection_string", standardStr),
+ )
+ }
l.Info(
"Using standard MongoDB connection string (no SRV lookup required)",
zap.String("cluster_name", clusterName),
zap.String("group_id", groupID),
)
return &connectionInfo{
scheme: "mongodb",
- hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.Standard, "mongodb://"),
+ hosts: strings.TrimPrefix(standardStr, mongoDBPrefix),
}, nil
}Apply similar validation for the SRV fallback path.
📝 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.
| // Prefer Standard (mongodb://) connection string as it doesn't require DNS SRV lookups. | |
| // This is important when using a SOCKS5 proxy since SRV lookups may not go through the proxy. | |
| if clusterInfo.ConnectionStrings.Standard != nil && *clusterInfo.ConnectionStrings.Standard != "" { | |
| l.Info( | |
| "Using standard MongoDB connection string (no SRV lookup required)", | |
| zap.String("cluster_name", clusterName), | |
| zap.String("group_id", groupID), | |
| ) | |
| return &connectionInfo{ | |
| scheme: "mongodb", | |
| hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.Standard, "mongodb://"), | |
| }, nil | |
| } | |
| return strings.TrimPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://"), nil | |
| // Fall back to SRV connection string if Standard is not available | |
| if clusterInfo.ConnectionStrings.StandardSrv != nil && *clusterInfo.ConnectionStrings.StandardSrv != "" { | |
| l.Warn( | |
| "Standard connection string not available, falling back to SRV (DNS lookups may not go through proxy)", | |
| zap.String("cluster_name", clusterName), | |
| zap.String("group_id", groupID), | |
| ) | |
| return &connectionInfo{ | |
| scheme: "mongodb+srv", | |
| hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://"), | |
| }, nil | |
| } | |
| // Prefer Standard (mongodb://) connection string as it doesn't require DNS SRV lookups. | |
| // This is important when using a SOCKS5 proxy since SRV lookups may not go through the proxy. | |
| if clusterInfo.ConnectionStrings.Standard != nil && *clusterInfo.ConnectionStrings.Standard != "" { | |
| const mongoDBPrefix = "mongodb://" | |
| standardStr := *clusterInfo.ConnectionStrings.Standard | |
| if !strings.HasPrefix(standardStr, mongoDBPrefix) { | |
| l.Warn("Unexpected standard connection string format", | |
| zap.String("connection_string", standardStr), | |
| ) | |
| } | |
| l.Info( | |
| "Using standard MongoDB connection string (no SRV lookup required)", | |
| zap.String("cluster_name", clusterName), | |
| zap.String("group_id", groupID), | |
| ) | |
| return &connectionInfo{ | |
| scheme: "mongodb", | |
| hosts: strings.TrimPrefix(standardStr, mongoDBPrefix), | |
| }, nil | |
| } | |
| // Fall back to SRV connection string if Standard is not available | |
| if clusterInfo.ConnectionStrings.StandardSrv != nil && *clusterInfo.ConnectionStrings.StandardSrv != "" { | |
| l.Warn( | |
| "Standard connection string not available, falling back to SRV (DNS lookups may not go through proxy)", | |
| zap.String("cluster_name", clusterName), | |
| zap.String("group_id", groupID), | |
| ) | |
| return &connectionInfo{ | |
| scheme: "mongodb+srv", | |
| hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://"), | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/mongodriver/mongo_driver.go` around lines 237 - 262, Validate
the connection string prefixes before trimming to avoid producing malformed
URIs: in the logic that returns &connectionInfo{...} for
clusterInfo.ConnectionStrings.Standard and StandardSrv, check
strings.HasPrefix(*clusterInfo.ConnectionStrings.Standard, "mongodb://") and
strings.HasPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://")
respectively before calling strings.TrimPrefix; if the expected prefix is
missing, return a clear error (including clusterName and groupID) instead of
trimming blindly so connectionInfo.scheme and hosts are always correct. Ensure
both branches (Standard and StandardSrv) perform the same validation and error
handling.
- Increase base dialer timeout from 30s to 60s - Add ConnectTimeout (60s) and ServerSelectionTimeout (60s) for MongoDB driver when using SOCKS5 proxy - SOCKS5 connections take longer due to: proxy connect -> SOCKS5 handshake -> proxy connects to target -> TLS handshake Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/connector/mongoconfig/proxy.go`:
- Around line 22-24: The Address method in MongoProxy uses fmt.Sprintf("%s:%d",
p.Host, p.Port) which produces invalid IPv6 addresses; change MongoProxy.Address
to use net.JoinHostPort with p.Host and strconv.Itoa(p.Port) (or fmt.Sprintf on
p.Port converted to string) so IPv6 hosts are bracketed correctly and the
address is generated safely.
In `@pkg/connector/mongodriver/mongo_driver.go`:
- Around line 235-240: The code dereferences clusterInfo.ConnectionStrings after
calling m.adminClient.ClustersApi.GetCluster (in mongo_driver.go), which can be
nil and cause a panic; add a nil-check right after obtaining clusterInfo (from
GetCluster/clusterInfo) and if ConnectionStrings is nil return a clear error
(e.g., fmt.Errorf("cluster %s connection strings missing for group %s",
clusterName, groupID)) so callers get an explicit failure instead of a panic.
♻️ Duplicate comments (1)
pkg/connector/mongodriver/mongo_driver.go (1)
241-265: Validate connection string prefixes before trimming.
strings.TrimPrefixdoesn’t verify the prefix, which can yield malformed URIs if Atlas returns an unexpected format.🐛 Proposed fix
if clusterInfo.ConnectionStrings.Standard != nil && *clusterInfo.ConnectionStrings.Standard != "" { + const mongoDBPrefix = "mongodb://" + standardStr := *clusterInfo.ConnectionStrings.Standard + if !strings.HasPrefix(standardStr, mongoDBPrefix) { + return nil, fmt.Errorf("unexpected standard connection string format for cluster %s", clusterName) + } l.Info( "Using standard MongoDB connection string (no SRV lookup required)", zap.String("cluster_name", clusterName), zap.String("group_id", groupID), ) return &connectionInfo{ scheme: "mongodb", - hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.Standard, "mongodb://"), + hosts: strings.TrimPrefix(standardStr, mongoDBPrefix), }, nil } if clusterInfo.ConnectionStrings.StandardSrv != nil && *clusterInfo.ConnectionStrings.StandardSrv != "" { + const mongoDBSrvPrefix = "mongodb+srv://" + srvStr := *clusterInfo.ConnectionStrings.StandardSrv + if !strings.HasPrefix(srvStr, mongoDBSrvPrefix) { + return nil, fmt.Errorf("unexpected SRV connection string format for cluster %s", clusterName) + } l.Warn( "Standard connection string not available, falling back to SRV (DNS lookups may not go through proxy)", zap.String("cluster_name", clusterName), zap.String("group_id", groupID), ) return &connectionInfo{ scheme: "mongodb+srv", - hosts: strings.TrimPrefix(*clusterInfo.ConnectionStrings.StandardSrv, "mongodb+srv://"), + hosts: strings.TrimPrefix(srvStr, mongoDBSrvPrefix), }, nil }
🧹 Nitpick comments (1)
pkg/connector/mongoconfig/proxy.go (1)
26-51: Context cancellation isn’t honored in the non-context dialer wrapper.When
socksDialerlacksDialContext, the wrapper ignoresctx. Consider a fast-fail check so deadlines/cancellations are respected before dialing.♻️ Proposed fix
// Wrap non-context dialer return contextDialerFunc(func(ctx context.Context, network, address string) (net.Conn, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } return socksDialer.Dial(network, address) }), nil }
| func (p *MongoProxy) Address() string { | ||
| return fmt.Sprintf("%s:%d", p.Host, p.Port) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "proxy.go" -path "*/mongoconfig/*" | head -20Repository: ConductorOne/baton-mongodb-atlas
Length of output: 112
🏁 Script executed:
cat -n pkg/connector/mongoconfig/proxy.go | head -40Repository: ConductorOne/baton-mongodb-atlas
Length of output: 1143
🏁 Script executed:
# Check the MongoProxy struct definition to understand the Port field type
ast-grep --pattern $'type MongoProxy struct {
$$$
}' pkg/connector/mongoconfig/proxy.goRepository: ConductorOne/baton-mongodb-atlas
Length of output: 277
🌐 Web query:
net.JoinHostPort IPv6 safe Go standard library documentation
💡 Result:
Yes — net.JoinHostPort is safe for IPv6. It returns "host:port" but will wrap the host in square brackets when the host contains a colon (i.e. an IPv6 literal), producing addresses like "[2001:db8::1]:80" or "[fe80::1%lo0]:53". Use net.JoinHostPort(host, port) to build addresses for Dial/listen. (pkg.go.dev)
Citations:
Use net.JoinHostPort for IPv6-safe proxy addresses.
fmt.Sprintf("%s:%d", ...) fails with IPv6 hosts by omitting required bracket wrapping (e.g., produces 2001:db8::1:8080 instead of [2001:db8::1]:8080). Use net.JoinHostPort instead, which is the idiomatic and safe formatter.
♻️ Proposed fix
func (p *MongoProxy) Address() string {
- return fmt.Sprintf("%s:%d", p.Host, p.Port)
+ return net.JoinHostPort(p.Host, fmt.Sprint(p.Port))
}📝 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.
| func (p *MongoProxy) Address() string { | |
| return fmt.Sprintf("%s:%d", p.Host, p.Port) | |
| } | |
| func (p *MongoProxy) Address() string { | |
| return net.JoinHostPort(p.Host, fmt.Sprint(p.Port)) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/mongoconfig/proxy.go` around lines 22 - 24, The Address method
in MongoProxy uses fmt.Sprintf("%s:%d", p.Host, p.Port) which produces invalid
IPv6 addresses; change MongoProxy.Address to use net.JoinHostPort with p.Host
and strconv.Itoa(p.Port) (or fmt.Sprintf on p.Port converted to string) so IPv6
hosts are bracketed correctly and the address is generated safely.
| clusterInfo, _, err := m.adminClient.ClustersApi.GetCluster(ctx, groupID, clusterName). | ||
| Execute() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get cluster: %w", err) | ||
| return nil, fmt.Errorf("failed to get cluster: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against nil ConnectionStrings to avoid panic.
clusterInfo.ConnectionStrings can be nil; dereferencing it will panic. Add a nil-check and return a clear error.
🐛 Proposed fix
clusterInfo, _, err := m.adminClient.ClustersApi.GetCluster(ctx, groupID, clusterName).
Execute()
if err != nil {
return nil, fmt.Errorf("failed to get cluster: %w", err)
}
+ if clusterInfo.ConnectionStrings == nil {
+ l.Error(
+ "Cluster connection strings missing",
+ zap.String("cluster_name", clusterName),
+ zap.String("group_id", groupID),
+ )
+ return nil, fmt.Errorf("cluster %s does not have connection strings", clusterName)
+ }🤖 Prompt for AI Agents
In `@pkg/connector/mongodriver/mongo_driver.go` around lines 235 - 240, The code
dereferences clusterInfo.ConnectionStrings after calling
m.adminClient.ClustersApi.GetCluster (in mongo_driver.go), which can be nil and
cause a panic; add a nil-check right after obtaining clusterInfo (from
GetCluster/clusterInfo) and if ConnectionStrings is nil return a clear error
(e.g., fmt.Errorf("cluster %s connection strings missing for group %s",
clusterName, groupID)) so callers get an explicit failure instead of a panic.
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Maybe we want a config option that is default true in lambda, false outside of lambda to only route through the proxy in this case?
| Port: cc.MongoProxyPort, | ||
| } | ||
|
|
||
| if mProxy.Port == 0 && mProxy.Host != "" { |
There was a problem hiding this comment.
Do we want to limit to proxy just for the lambda? We probably don't want it in service mode?
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.