-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Specify a custom dial function per config #1527
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe recent updates introduce a significant enhancement in connection management by incorporating a configurable Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- connector.go (1 hunks)
- dsn.go (1 hunks)
Additional comments: 6
connector.go (3)
80-92: Please ensure to add tests that cover the new functionality introduced by the use of
c.cfg.DialFunc
. This is crucial for verifying that the custom dial function works as expected and handles errors properly.80-92: Remember to update the documentation to include information about the new
DialFunc
configuration option. This will help users understand how to use the new feature.80-92: If this is your first contribution to the project, please add yourself to the AUTHORS file as per the project's contribution guidelines.
dsn.go (3)
55-55: Verify whether the
DialFunc
field should be included in theFormatDSN
method output. If it is intended to be part of the DSN string, appropriate handling needs to be added.55-55: Confirm that the
DialFunc
field does not need to be normalized within thenormalize
method. If it does require normalization, appropriate logic should be added.55-55: Ensure that the
DialFunc
field does not need to be parsed from the DSN string in theparseDSNParams
function. If it should be included, parsing logic will need to be implemented.
connector.go
Outdated
dctx, cancel = context.WithTimeout(ctx, c.cfg.Timeout) | ||
defer cancel() | ||
} | ||
mc.netConn, err = c.cfg.DialFunc(dctx, mc.cfg.Addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a potential issue with error handling after the custom DialFunc
is called. If an error occurs, it should be handled similarly to how it's handled after the global dial
function or net.Dialer
is called.
+ mc.netConn, err = c.cfg.DialFunc(dctx, mc.cfg.Addr)
+ if err != nil {
+ return nil, err
+ }
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.
mc.netConn, err = c.cfg.DialFunc(dctx, mc.cfg.Addr) | |
mc.netConn, err = c.cfg.DialFunc(dctx, mc.cfg.Addr) | |
if err != nil { | |
return nil, err | |
} |
df2bedf
to
ac7765c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- connector.go (1 hunks)
- dsn.go (2 hunks)
Additional comments: 5
connector.go (1)
- 84-91: The implementation of the conditional dial function usage looks correct. It checks if a custom dial function is provided and uses it to establish a connection. If not, it falls back to the previously existing dialing mechanisms. This change aligns with the PR's objective to allow specifying a custom dial function for each configuration.
However, ensure that the custom
DialFunc
is thoroughly tested, especially since the PR checklist indicates that tests for this new functionality are pending. It's important to cover various scenarios, including timeouts and error handling, to ensure the new dialing mechanism works as expected.dsn.go (4)
13-13: The import of the
context
package is appropriate given the newDialFunc
field in theConfig
struct.70-71: The addition of the
DialFunc
field to theConfig
struct enables per-configuration custom dial functions, which is a key objective of this PR.70-71: The function signature for
DialFunc
is correct and follows the conventional pattern for dial functions in Go, taking acontext.Context
, network type, and address, and returning anet.Conn
and an error.74-74: Verify if any normalization is required for the new
DialFunc
field within thenormalize
method.
ac7765c
to
2d38b27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- connector.go (1 hunks)
- dsn.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- connector.go
- dsn.go
2d38b27
to
f0f5dd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- connector.go (1 hunks)
- dsn.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- connector.go
- dsn.go
f0f5dd4
to
31d874d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- connector.go (1 hunks)
- dsn.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- connector.go
- dsn.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- connector.go (1 hunks)
- dsn.go (1 hunks)
Additional comments: 3
connector.go (2)
- 90-97: The implementation of the custom
DialFunc
usage within theConnect
method is correctly checking ifDialFunc
is provided and uses it if available. This approach allows for flexibility in establishing network connections, aligning with the PR's objectives. However, consider adding a comment explaining the purpose of the conditionalcontext.WithTimeout
creation based onmc.cfg.Timeout
to clarify its intent for future maintainers.- 99-113: The fallback mechanism to use the default dialing method when
DialFunc
is not provided is correctly implemented. However, there's a past comment bymethane
suggesting not to lookupdials
whenc.cfg.DialFunc != nil
. This has been addressed by the conditional check forc.cfg.DialFunc
at the beginning of this segment. It's good practice to ensure that unnecessary operations are avoided when a custom dial function is provided, enhancing performance and maintainability.dsn.go (1)
- 57-57: The addition of the
DialFunc
field to theConfig
struct is correctly implemented, allowing users to specify a custom dial function for connection establishment. This change effectively addresses the PR's objectives by enhancing flexibility in connection handling. It's important to ensure that documentation is updated to include information about this new field and how to use it, as it introduces a significant new capability for users.
mc.parseTime = mc.cfg.ParseTime | ||
|
||
// Connect to Server | ||
dialsLock.RLock() | ||
dial, ok := dials[mc.cfg.Net] | ||
dialsLock.RUnlock() | ||
if ok { | ||
if c.cfg.DialFunc != nil { | ||
dctx := ctx | ||
if mc.cfg.Timeout > 0 { | ||
var cancel context.CancelFunc | ||
dctx, cancel = context.WithTimeout(ctx, c.cfg.Timeout) | ||
defer cancel() | ||
} | ||
mc.netConn, err = dial(dctx, mc.cfg.Addr) | ||
mc.netConn, err = c.cfg.DialFunc(dctx, mc.cfg.Net, mc.cfg.Addr) | ||
} else { | ||
nd := net.Dialer{Timeout: mc.cfg.Timeout} | ||
mc.netConn, err = nd.DialContext(ctx, mc.cfg.Net, mc.cfg.Addr) | ||
dialsLock.RLock() | ||
dial, ok := dials[mc.cfg.Net] | ||
dialsLock.RUnlock() | ||
if ok { | ||
dctx := ctx | ||
if mc.cfg.Timeout > 0 { | ||
var cancel context.CancelFunc | ||
dctx, cancel = context.WithTimeout(ctx, c.cfg.Timeout) | ||
defer cancel() | ||
} | ||
mc.netConn, err = dial(dctx, mc.cfg.Addr) | ||
} else { | ||
nd := net.Dialer{Timeout: mc.cfg.Timeout} | ||
mc.netConn, err = nd.DialContext(ctx, mc.cfg.Net, mc.cfg.Addr) | ||
} | ||
} | ||
|
||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [175-179]
The TLS configuration used in the default and custom dial functions does not specify a minimum version. It's recommended to explicitly set the minimum TLS version to TLS 1.3 to ensure stronger security guarantees. Additionally, the use of InsecureSkipVerify: true
in the skip-verify
case should be carefully considered, as it disables TLS/SSL certificate verification, making the connection vulnerable to man-in-the-middle (MTM) attacks. This should only be used for debugging purposes and not in production environments.
- cfg.TLS = &tls.Config{InsecureSkipVerify: true}
+ cfg.TLS = &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS13}
And for other relevant TLS configurations, ensure MinVersion: tls.VersionTLS13
is added.
I would like to see in the description of this PR an example of how that new feature would be used. Because I would to ensure this can't be implemented in another way. |
I’m not quite sure what kind of example you want. I think my PR description has already explained it clearly. Without using different custom net names, RegisterDialContext can only register the SSH Tunnel globally. However, what I want is to be able to use multiple SSH Tunnels in one program without registering the net globally. In this PR, different Tunnels can be configured through DSN configuration. |
Description
Specify a custom dial function per config instead of using RegisterDialContext. It's hard to bind different state infomation for dialing with
RegisterDialContext
. Like SSH tunneling, if we want to use diffrent SSH connections, uniquenet
for RegisterDialContext is required If I'm not wrong.Use cases:
Config
/dsn
Checklist
Summary by CodeRabbit
connector
component to support flexible connection establishment through a newDialFunc
option.Config
struct documentation to reflect the addition of theDialFunc
field, enabling more controlled connection processes.