Skip to content
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

Allow passing a custom client-side dialer #80

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

brandond
Copy link
Member

@brandond brandond commented Jul 2, 2024

Issue:

Problem

Our remotedialer client needs to be able to provide a custom dialer for use when dialing connections on behalf of the server. Currently, the client dialer always uses an unconfigured net.Dialer{} struct, because there is no code path to call NewClientSessionWithDialer, meaning that s.dialer is always nil:

  • func (s *Session) clientConnect(ctx context.Context, message *message) error {
    if s.auth == nil || !s.auth(message.proto, message.address) {
    return errors.New("connect not allowed")
    }
    conn := newConnection(message.connID, s, message.proto, message.address)
    s.addConnection(message.connID, conn)
    go clientDial(ctx, s.dialer, conn, message)
    return nil
    }
  • if dialer == nil {
    d := net.Dialer{}
    netConn, err = d.DialContext(ctx, message.proto, message.address)
    } else {
    netConn, err = dialer(ctx, message.proto, message.address)
    }

Solution

Add ConnectToProxyWithDialer that calls NewClientSessionWithDialer so that callers can provide a custom local dialer function.

There is no change to the existing ConnectoToProxy function signature or behavior, as the current code path also just calls NewClientSessionWithDialer(auth, ws, nil)

CheckList

  • Test

Add ConnectToProxyWithDialer that calls NewClientSessionWithDialer so that callers can provide a custom local dialer function. The current code always calls NewClientSession, which uses a default net.Dialer that cannot be configured.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - changes look backward compatible with the current set of functions.

@MbolotSuse MbolotSuse merged commit afe9785 into rancher:master Jul 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants