Skip to content

[Connection] Add an optional Dialer to ClientOptions - #1532

Open
SimonWoolf wants to merge 1 commit into
apache:masterfrom
ably-forks:ably-dialer
Open

[Connection] Add an optional Dialer to ClientOptions#1532
SimonWoolf wants to merge 1 commit into
apache:masterfrom
ably-forks:ably-dialer

Conversation

@SimonWoolf

Copy link
Copy Markdown

Motivation

There is currently no way for a caller to influence how the client establishes TCP connections to brokers: connection.connect() calls net.Dial/net.DialTimeout/tls.DialWithDialer directly, and ClientOptions exposes no hook. That makes several things impossible from outside the library (in our case, we want to blocklist IPs in a way that isn't trivially avoidable by having a bad actor change DNS resolution for an address they supply after the pulsar producer is constructed).

Modifications

This adds an optional ClientOptions.Dialer.

connect() is restructured so the TCP dial and the TLS handshake are separate steps. tls.DialWithDialer does both at once and cannot take an injected dialer, so it becomes a dial followed by tls.Client(...).Handshake(). The dialer therefore returns a plain net.Conn and the library still owns TLS, leaving getTLSConfig() and certificate verification unchanged.

(NB: tls.DialWithDialer infers ServerName from the address being dialed when the config leaves it blank, whereas tls.Client does not and would verify against an empty name. This preserves that behaviour).

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows: go test ./pulsar/internal/ -run TestConnection

The existing TLS integration tests cover the restructured connection.connect().

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: yes, in a non-breaking way (adds a client option)
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no

Documentation

  • Does this pull request introduce a new feature? yes (the new dialer)
  • If yes, how is the feature documented? GoDocs

There is currently no way for a caller to influence how the client
establishes TCP connections to brokers: connection.connect() calls
net.Dial/net.DialTimeout/tls.DialWithDialer directly, and ClientOptions
exposes no hook. That makes several things impossible from outside the
library (in our case, we want to blocklist IPs in a way that isn't
trivially avoidable by having a bad actor change DNS resolution for
an address they supply after the pulsar producer is constructed).

This adds an optional ClientOptions.Dialer.

connect() is restructured so the TCP dial and the TLS handshake are separate
steps. tls.DialWithDialer does both at once and cannot take an injected
dialer, so it becomes a dial followed by tls.Client(...).HandshakeContext().
The dialer therefore returns a plain net.Conn and the library still owns TLS,
leaving getTLSConfig() and certificate verification unchanged.

Two behaviours of tls.DialWithDialer are preserved explicitly, since
tls.Client does not provide them:

- ServerName is inferred from the address being dialed when the config
  leaves it blank. tls.Client would otherwise verify against an empty
  name and fail.

- ConnectionTimeout bounds the dial and the handshake together, not just
  the dial. The context is therefore built once in connect() and passed
  to both, mirroring what crypto/tls does internally. Bounding only the
  dial would let a peer that accepts TCP and then goes silent hang the
  connection attempt indefinitely.

HandshakeContext is used rather than setting a deadline on the raw conn,
so no deadline lingers on the long-lived broker connection afterwards.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant